[Outdated, see #13790][CPU] StackAllocator for JIT kernels#13132
[Outdated, see #13790][CPU] StackAllocator for JIT kernels#13132lohika-denis-kotov wants to merge 20 commits into
Conversation
lohika-denis-kotov
changed the base branch from
feature/stack_allocator
to
master
|
@ceciliapeng2011 @maxnick @nshchego Please, review this PR |
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
from
216f782 to
ccc73e6
Compare
ceciliapeng2011
requested review from
dmitry-gorokhov,
maxnick and
nshchego
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
from
ccc73e6 to
18b15a4
Compare
|
|
||
| StackAllocator(x64::jit_generator& code_gen, | ||
| const Xbyak::Reg& bp, | ||
| const size_t alignment) |
There was a problem hiding this comment.
When do we need the stack aligned > 1?
There was a problem hiding this comment.
Yes, for example most of sse4.1 commands requires address to be aligned
There was a problem hiding this comment.
@ceciliapeng2011 Do you have some questions related to alignment ?
If no, please resolve conversation
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
from
755ed8d to
712bb2d
Compare
lohika-denis-kotov
requested review from
OleksiiZaderykhinCapgemini and
ceciliapeng2011
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
from
9f997e0 to
8a1144a
Compare
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
2 times, most recently
from
6e38ad5 to
8fd8b0b
Compare
|
@maxnick @dmitry-gorokhov @ceciliapeng2011 @nshchego Could you review this PR ? |
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
from
c29da37 to
fb0ac22
Compare
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
from
fb0ac22 to
86e7813
Compare
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
3 times, most recently
from
4f56dea to
670934d
Compare
…t operator bool()
lohika-denis-kotov
force-pushed
the
feature/stack_allocator
branch
from
670934d to
99650b4
Compare
maxnick
left a comment
maxnick
left a comment
There was a problem hiding this comment.
In general LGTM, we need to resolve a few comment regarding namespace usage and class names.
| namespace intel_cpu { | ||
|
|
||
| using namespace dnnl::impl::cpu; | ||
| namespace x64 = dnnl::impl::cpu::x64; |
There was a problem hiding this comment.
As far as I can see only x64::cpu_isa_t is used. M.b. it does make sense to use an alias for this type on the RegistersPool level:
class RegistersPool {
public:
using cpu_isa_t = dnnl::impl::cpu::x64::cpu_isa_t;So, within this header it can be accessed just as follows RegistersPool::cpu_isa_t
What do you think?
There was a problem hiding this comment.
We also have other references to x64 namespace in this header like x64::cpu_isa_traits and cpu_isa_t enum members.
There was a problem hiding this comment.
Do not see an elegant solution here, since the whole class is intended to be used with dnnl::impl::cpu::x64::cpu_isa_t members, and we do not have a possibility to import enumeration to the class (it has been introduced in c++20). I guess we should think about wrapping some dnnl entities on jit_base level.
lohika-vmostovy
changed the title
|
Closed as outdated |
6 participants
Description
StackAllocator that helps to allocate memory on stack with some automatic deallocation
Edit by @lohika-vmostovy :
I've created a new PR #13790 because I can't modify this one in order to address comments. Please, leave new comments in a new PR.