Closed Bug 1116491 Opened 5 years ago Closed 5 years ago

IonMonkey should keep the largest code alignment of the Macro Assembler.

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently, with the latest patch attached on Bug 1112154, the MSimdConstant production out of the constructor is disabled.  The reason is that this is producing a SIGSEGV which is produced by the lack of alignment as reported by gdb:

(gdb) x /i $pc
=> 0x7ffff7e4b936:      movaps 0xa0ab(%rip),%xmm1        # 0x7ffff7e559e8

The Macro Assembler well asserts that we do correctly align the address when we spill the simd constants at the end of the Macro Assembler buffer (in the finish function).  So the problem is likely caused by the copy of the code buffer which does not keep the alignment of SIMD instructions.

Note, that if this is the case, then we probably have a similar issue for any code alignment logic which is done in the codegen.
This patch updates the CodeAlignment constant such that SIMD constants which
are baked in the JitCode of IonMonkey are well aligned, as expected during
the encoding of the macro assembler buffer.
Attachment #8543289 - Flags: review?(benj)
Comment on attachment 8543289 [details] [diff] [review]
IonMonkey: Use a larger code alignment on x86/x64 to load SIMD constants from code sections. r=

Review of attachment 8543289 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find! I'd rather
- not introduce a new constant in Assembler-x* files and reuse SimdStackAlignment (agreed if you rename it into SimdAlignment or SimdMemAlignment or SimdMemoryAlignment or SimdDataAlignment)
- static_assert(CodeAlignment => SimdDataAlignment), with your comment next to this static assertion
Attachment #8543289 - Flags: review?(benj)
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> - static_assert(CodeAlignment => SimdDataAlignment)

and of course, that's >= rather than => (even if, in this particular case, "implies" makes sense :))
Delta:
 - Rename SimdStackAlignment to SimdMemoryAlignment
 - Add an assertion which ensure that Code sections in which SIMD constants are
   added are well aligned.
 - Increase x86 & x64 CodeAlignment to match the previous assertion.
Attachment #8543289 - Attachment is obsolete: true
Attachment #8544579 - Flags: review?(benj)
Delta:
 - Fix typos in the static_assert comment.
Attachment #8544579 - Attachment is obsolete: true
Attachment #8544579 - Flags: review?(benj)
Attachment #8544582 - Flags: review?(benj)
Comment on attachment 8544582 [details] [diff] [review]
IonMonkey: Use a larger code alignment on x86/x64 to load SIMD constants from code sections. r=

Review of attachment 8544582 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks!

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +310,5 @@
>                case MIRType_Float32:
>                  masm.storeDouble(ToFloatRegister(ins->arg()), dst);
>                  return;
> +              // StackPointer is SimdMemoryAlignment-aligned and ABIArgGenerator guarantees stack
> +              // offsets are SimdMemoryAlignment-aligned.

while you're here: s/SimdMemoryAlignment-aligned/SIMD-aligned in these last two lines
Attachment #8544582 - Flags: review?(benj) → review+
https://hg.mozilla.org/mozilla-central/rev/171a1e7209a0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.