[CPU] StackAllocator for JIT kernels#13790
Conversation
lohika-vmostovy
requested review from
OleksiiZaderykhinCapgemini,
ceciliapeng2011,
dmitry-gorokhov,
maxnick and
nshchego
There was a problem hiding this comment.
I have a time to comment on this renaming ...
As for me this name RegAddress raise more questions ...
What does it mean to have address of register ?
This renaming provide inconsistency with RegisterPool::Reg<...>
With previous name it was possible to write the general helper function like this:
template <typename TAllocator, typename TReg>
void custom_gather(TAllocator::Reg<TReg>& arg0, ...) {
...
}But with such naming convention it is not possible ...
Anyway as for me it is enough to have prefix before class name that describe where it was allocated
There was a problem hiding this comment.
What does it mean to have address of register ?
This means the address of a data block of a certain register type size.
With previous name it was possible to write the general helper function like this:
The problem is that the StackAllocator::Reg is not the same entity as RegisterPool::Reg since it is not a hardware register, but an memory address, with completely different access metrics (latency could be equal to DRAM access time in a worst case), so it should not be used interchangeably by default, but only intentionally when the developer understand all the drawbacks. So such generic templated code may lead to unexpected performance drop due to memory access instead of register, but it would not be obvious from the source code. That is why, at least in my understanding, we should highlight this difference using slightly different name.
There was a problem hiding this comment.
I think StackAllocator:: prefix is enough.
Anyway as for me this renaming is very subjective ...
There was a problem hiding this comment.
Such change make it inconsistent with namespace x64 = dnnl::impl::cpu::x64; in RegisterPool file
There was a problem hiding this comment.
Actually that would be great to get read of this alias in RegisterPool as well, just to avoid possible namespace conflicts, but in case of RegisterPool it is so tightly coupled with cpu_isa_t, that we should leave it for a while ad try to address this issue on the jit_base level.
lohika-vmostovy
force-pushed
the
feature/dekotov_stack_allocator
branch
from
d182e8a to
8291200
Compare
lohika-vmostovy
force-pushed
the
feature/dekotov_stack_allocator
branch
from
8291200 to
0a6d50a
Compare
|
@maxnick ci/jenkins, build_macos_release and build_ubuntu20_release jobs are failing. I have no permissions to see logs and try to fix them. Could you please take a look on those jobs and give me some insights? |
lohika-vmostovy
force-pushed
the
feature/dekotov_stack_allocator
branch
from
0a6d50a to
68e5f7b
Compare
@lohika-vmostovy I will help on this build. It is a known issue of CI, fixing is on the way. Will let you know once it is merged. Yaroslav can also get CI logs and retirgger via VPN. |
lohika-vmostovy
force-pushed
the
feature/dekotov_stack_allocator
branch
from
68e5f7b to
b8f94d5
Compare
…t operator bool()
lohika-vmostovy
force-pushed
the
feature/dekotov_stack_allocator
branch
from
b8f94d5 to
7546c42
Compare
|
This PR will be closed in 2 weeks in case of no activity. |
|
This PR was closed because it has been stalled for 2 week with no activity. |
8 participants
This PR is a clone of #13132 and is intended to be merged instead, because we don't have an option to modify original PR.
All commits from original PR are preserved without any modification.
Additional commits are added on top in order to address PR comments.