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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Use a larger code alignment on x86/x64 to load SIMD constants from code sections. r=

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.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

(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 :))

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Use a larger code alignment on x86/x64 to load SIMD constants from code sections.

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.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Use a larger code alignment on x86/x64 to load SIMD constants from code sections. r=

Delta:
 - Fix typos in the static_assert comment.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 7

[Nicolas B. Pierron [:nbp]]

(try) https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=88073c17bfa9
(inbound) https://hg.mozilla.org/integration/mozilla-inbound/rev/171a1e7209a0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/171a1e7209a0