Closed Bug 1435185 Opened 5 years ago Closed 5 years ago

[MIPS32] Ensure that Baldr and Rabaldr locals are naturally aligned

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36
Attached patch rabaldr.patch (obsolete) — Splinter Review
This issue is present on 32-bit targets since Bug 1411800 shrunk the size of wasm::Frame to 3 pointers making the bottom of stack locals only word aligned. This will primarily affect mips32 when we switch to doubleword load and store instructions for double locals which trap on unaligned access. 

For baseline I chose to align bottom of locals area to WasmStackAlignment on all platforms given that existing implementation tries to align size of locals area to WasmStackAlignment. Is this a correct approach to this issue, or maybe should I just change layout of wasm::Frame to include padding on 32-bit archs (all of them, mips32) ? I'm not sure that it matters that much but WasmStackAlignment might be a bit overzealous on x86 since there are no simd types in baseline.
Flags: needinfo?(lhansen)
Interesting point.  Since ARM requires only 4-byte alignment for doubles and int64, and we don't have atomics on the stack, we never had a chance to discover this problem yet.

I think the right fix is actually to add a padding word to wasm::Frame.  I need to do that anyway for ARM64 (where the stack is always 16-aligned).  In my current patch queue I have added it (#ifdef ARM64) between tls and returnAddress, like so:

------ 8< ------------------------------------------------
@@ -1865,6 +1865,10 @@ struct Frame
     // effectively the callee's instance.
     TlsData* tls;
 
+#ifdef JS_CODEGEN_ARM64
+    void* padding;
+#endif
+
     // The return address pushed by the call (in the case of ARM/MIPS the return
     // address is pushed by the first instruction of the prologue).
     void* returnAddress;
@@ -1874,6 +1878,10 @@ struct Frame
     Instance* instance() const { return tls->instance; }
 };
 
+#ifdef JS_CODEGEN_ARM64
+static_assert(sizeof(Frame) % 16 == 0, "frame size");
+#endif
+
 // A DebugFrame is a Frame with additional fields that are added after the
 // normal function prologue by the baseline compiler. If a Module is compiled
 // with debugging enabled, then all its code creates DebugFrames on the stack
------ 8< ------------------------------------------------

This seems to work OK but my port is not complete yet so I can't be absolutely sure.

Don't know if we should just do this on all platforms or on the ones that need it.  Let's ask Luke what he thinks.
Flags: needinfo?(lhansen) → needinfo?(luke)
Could we keep Frame the same size and instead factor the padding into the existing stack adjustment that happens after the prologue?  Adding padding into the middle of the Frame will add code to the prologue/epilogue size and add complexity to StartUnwinding().  Ion codegen already does similar padding (for SIMD, which requires 16-byte alignment) when computing the 'framePushed' that is passed to GenerateFunctionPrologue().  (See the CodeGeneratorShared() ctor in jit/shared/CodeGenerator-shared.cpp and its use of WasmSTackAlignment.)  It seems like we should do likewise for rabaldr.
Flags: needinfo?(luke)
(In reply to Luke Wagner [:luke] from comment #3)
> Could we keep Frame the same size and instead factor the padding into the
> existing stack adjustment that happens after the prologue?  Adding padding
> into the middle of the Frame will add code to the prologue/epilogue size and
> add complexity to StartUnwinding().  Ion codegen already does similar
> padding (for SIMD, which requires 16-byte alignment) when computing the
> 'framePushed' that is passed to GenerateFunctionPrologue().  (See the
> CodeGeneratorShared() ctor in jit/shared/CodeGenerator-shared.cpp and its
> use of WasmSTackAlignment.)  It seems like we should do likewise for rabaldr.

Hi, 

I'm ok with both ways. You answer kinda implies that this should be done for all platforms, but if we plan to do this only for arm64 and mips32 please note that mips32 already has a separate codepath in StartUnwinding and I have a local patch that gets
rid of push and pop sequences on wasm prologue/epilogue for mips32/mips64 (as I see now almost the same to what @lhansen did to arm64 at least in prologue), so we might get alignment for both platforms with no code size increase and no more complexity in WasmFrameIter than it already is or will be soonish (some codepaths might get merged) ?
Do you think that this should be done for all platforms? If so, should be alignment be WasmStackAlignment  or something more lax
WasmLocalsAligment? that would be 8 bytes for x64 (no-op) and x86 ?
Flags: needinfo?(luke)
Not answering for Luke here, but ARM64 has all sorts of differences from the standard code right now and some of those differences I will likely not be able to get rid of.  Keeping the Frame the same size (three words) is not an attractive option on ARM64 since we can then no longer use SP to initialize the frame during the prologue as SP will be invalid (not divisible by 16 when used for memory references); atm the code subtracts 32 bytes and stores three pointers off the SP, above it.  And as for MIPS, the unwinding code is somewhat different because we dealloc the frame all at once.  (And as I've pointed out before, the Frame layout ought to be a little more flexible so that we can use eg STM/LDM on ARM, which speaks in favor of a three-word Frame on that architecture, as on x86 and x64.)
Ah ok, it sounds like adding the trailing padding will solve multiple problems at once, then.  (Sorry I missed that before in the original comment.)  As for x86/x64, it seems attractive to keep Frames symmetric, but I can't think of any specific benefits and, on the downside, it'll add code size and more unwind logic, so having the padding be MIPS|ARM64 sounds like the right option.
Flags: needinfo?(luke)
Attached patch mips32_align.patch (obsolete) — Splinter Review
Patch that changes size of wasm::Frame on mips32.
Attachment #8948637 - Flags: feedback?(luke)
Attachment #8948637 - Flags: feedback?(lhansen)
Comment on attachment 8948637 [details] [diff] [review]
mips32_align.patch

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

Sure, it fits the structure I use for ARM64, so I'm fine with this.
Attachment #8948637 - Flags: feedback?(lhansen) → feedback+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Comment on attachment 8948637 [details] [diff] [review]
mips32_align.patch

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

::: js/src/wasm/WasmTypes.h
@@ +1868,5 @@
>      // effectively the callee's instance.
>      TlsData* tls;
>  
> +#if defined(JS_CODEGEN_MIPS32)
> +    uintptr_t padding_;

Before landing, we should have a comment explaining that some archs require double-worrd alignment and having this in the frame simplifies codegen.

@@ +1926,1 @@
>      uint32_t padding_;  // See alignmentStaticAsserts().

Just to confirm: did alignmentStaticAsserts() statically fail for you without this change?
Attachment #8948637 - Flags: feedback?(luke) → feedback+
(In reply to Luke Wagner [:luke] from comment #9)
> Comment on attachment 8948637 [details] [diff] [review]
> mips32_align.patch
> 
> @@ +1926,1 @@
> >      uint32_t padding_;  // See alignmentStaticAsserts().
> 
> Just to confirm: did alignmentStaticAsserts() statically fail for you
> without this change?

Yes.
Summary: Ensure that Baldr and Rabaldr locals are naturally aligned → [MIPS32] Ensure that Baldr and Rabaldr locals are naturally aligned
Attached patch bug1435185.patch (obsolete) — Splinter Review
Attachment #8947736 - Attachment is obsolete: true
Attachment #8948637 - Attachment is obsolete: true
Attachment #8948937 - Flags: review?(luke)
Comment on attachment 8948937 [details] [diff] [review]
bug1435185.patch

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

::: js/src/wasm/WasmTypes.h
@@ +1869,5 @@
>      TlsData* tls;
>  
> +#if defined(JS_CODEGEN_MIPS32)
> +    // See Bug 1435185.
> +    // Double word aligned frame ensures correct alignment for wasm locals.

Personally, I like to avoid bug #'s in comments since it implies there is some terrible obscure corner case, rather then a general principle that can be easily stated in the comment.  Here, I think we can capture the general principle by extending the sentence to say " on architectures that require the stack alignment to be twice the word size" and thus delete the "See Bug 1435185".
Attachment #8948937 - Flags: review?(luke) → review+
Attachment #8948937 - Attachment is obsolete: true
Attachment #8949018 - Flags: review+
Keywords: checkin-needed
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f302023b7f5
[MIPS32] [MIPS32] Ensure that Baldr and Rabaldr locals are naturally aligned ; r=luke
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f302023b7f5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee: nobody → dragan.mladjenovic
You need to log in before you can comment on or make changes to this bug.