Closed
Bug 1130089
Opened 9 years ago
Closed 9 years ago
Clean-up: factor definitions of '' JitStackAlignment / sizeof(Value); ''
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(1 file)
18.98 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
The alignment patches are adding the same computation as a neat trick to remove paths at compile time. We should move this definition to a header, and use MOZ_CONSTEXPR_VAR to define it.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8580906 -
Flags: review?(benj)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
Comment on attachment 8580906 [details] [diff] [review] Use constexpr for JitStackValueAlignment. Review of attachment 8580906 [details] [diff] [review]: ----------------------------------------------------------------- Nice ::: js/src/jit/LIR.h @@ +1731,5 @@ > // the number of slots per Value. > uint32_t paddedLocalSlotCount() const { > // Round to ABIStackAlignment, but also round to at least sizeof(Value) > // in case that's greater, because StackOffsetOfPassedArg rounds > // argument slots to 8-byte boundaries. comment is now stall, please update to say that JitStackAlignment is a multiple of sizeof(Value) ::: js/src/jit/Lowering.cpp @@ +393,5 @@ > // Align the arguments of a call such that the callee would keep the same > // alignment as the caller. > uint32_t baseSlot = 0; > + if (JitStackValueAlignment > 1) > + baseSlot = AlignBytes(argc, JitStackValueAlignment); feel free to make this a ternary, if it fits in 100 chars ::: js/src/jit/arm/Assembler-arm.h @@ +151,5 @@ > +static MOZ_CONSTEXPR_VAR uint32_t JitStackAlignment = 8; > + > +static MOZ_CONSTEXPR_VAR uint32_t JitStackValueAlignment = JitStackAlignment / sizeof(Value); > +static_assert(JitStackAlignment % sizeof(Value) == 0 && JitStackValueAlignment >= 1, > + "Stack alignment should be larger than a Value"); same as in assembler x64 ::: js/src/jit/mips/Assembler-mips.h @@ +166,5 @@ > +static MOZ_CONSTEXPR_VAR uint32_t JitStackAlignment = 8; > + > +static MOZ_CONSTEXPR_VAR uint32_t JitStackValueAlignment = JitStackAlignment / sizeof(Value); > +static_assert(JitStackAlignment % sizeof(Value) == 0 && JitStackValueAlignment >= 1, > + "Stack alignment should be larger than a Value"); same as in assembler x64 ::: js/src/jit/x64/Assembler-x64.h @@ +183,5 @@ > +static MOZ_CONSTEXPR_VAR uint32_t JitStackAlignment = 16; > + > +static MOZ_CONSTEXPR_VAR uint32_t JitStackValueAlignment = JitStackAlignment / sizeof(Value); > +static_assert(JitStackAlignment % sizeof(Value) == 0 && JitStackValueAlignment >= 1, > + "Stack alignment should be larger than a Value"); not only larger, but also a multiple ::: js/src/jit/x64/Trampoline-x64.cpp @@ +404,5 @@ > "No need to consider the JitFrameLayout for aligning the stack"); > static_assert(JitStackAlignment % sizeof(Value) == 0, > "Ensure that we can pad the stack by pushing extra UndefinedValue"); > > + MOZ_ASSERT(IsPowerOfTwo(JitStackValueAlignment)); Can it be made an static assert, ooc? I suspect no, because IsPowerOfTwo might not be applicable to known constants... ::: js/src/jit/x86/Assembler-x86.h @@ +113,5 @@ > +static MOZ_CONSTEXPR_VAR uint32_t JitStackAlignment = 16; > + > +static MOZ_CONSTEXPR_VAR uint32_t JitStackValueAlignment = JitStackAlignment / sizeof(Value); > +static_assert(JitStackAlignment % sizeof(Value) == 0 && JitStackValueAlignment >= 1, > + "Stack alignment should be larger than a Value"); same as in assembler x64
Attachment #8580906 -
Flags: review?(benj) → review+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #2) > ::: js/src/jit/x64/Trampoline-x64.cpp > @@ +404,5 @@ > > "No need to consider the JitFrameLayout for aligning the stack"); > > static_assert(JitStackAlignment % sizeof(Value) == 0, > > "Ensure that we can pad the stack by pushing extra UndefinedValue"); > > > > + MOZ_ASSERT(IsPowerOfTwo(JitStackValueAlignment)); > > Can it be made an static assert, ooc? I suspect no, because IsPowerOfTwo > might not be applicable to known constants... No because IsPowerOfTwo is not a constexpr, and recently I noticed that gcc 4.7 has issues with constexpr templates. So, not yet :(
Assignee | ||
Comment 4•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e1dd959091 https://treeherder.mozilla.org/#/jobs?repo=try&revision=054b26094100
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c2e1dd959091
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•