Closed Bug 1432446 Opened 2 years ago Closed 2 years ago

[MIPS] Reduce size of switch table entries to a single pointer

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

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

References

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 bug1432446.patch (obsolete) — Splinter Review
This patch changes switch table entries from being absolute jumps to case labels to being pointers to case labels like on other platforms. Unlike other platforms mips32/mips64 assembler needs to bind CodeLabel both as instruction immediate and as raw pointer in case of table entries. This patch also adds necessary plumbing to support that.
Depends on: wasm-mips
Attachment #8944683 - Flags: review?(bbouvier)
Comment on attachment 8944683 [details] [diff] [review]
bug1432446.patch

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

Sorry, I don't understand this code enough to give a review here.

Lars maybe?
Attachment #8944683 - Flags: review?(bbouvier) → review?(lhansen)
Assignee: nobody → dragan.mladjenovic
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8944683 [details] [diff] [review]
bug1432446.patch

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

I guess the patch looks OK, it doesn't affect non-mips at all, really, except by churn.  Not sure I like the churn, but, you can't always get what you want.

::: js/src/jit/arm64/Assembler-arm64.h
@@ +249,5 @@
>  
>      void processCodeLabels(uint8_t* rawCode) {
>          for (size_t i = 0; i < codeLabels_.length(); i++) {
>              CodeLabel label = codeLabels_[i];
> +            Bind(rawCode, *label.patchAt(), *label.target(), label.patchAtIsRawPointer());

"patchIsRawPointer()"?

@@ +255,5 @@
>      }
>  
> +    static void Bind(uint8_t* rawCode, CodeOffset label, CodeOffset address,
> +                     bool labelIsRawPointer) {
> +        MOZ_CRASH(labelIsRawPointer);

What is this MOZ_CRASH line doing?  Probably MOZ_ASSERT.

::: js/src/jit/shared/Assembler-shared.h
@@ +496,1 @@
>  // A code label contains an absolute reference to a point in the code. Thus, it

Spurious blank line.

@@ +507,5 @@
>      // get patched to.
>      CodeOffset target_;
>  
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +    bool patchAsRawPointer_;

This needs a comment to explain why it is here.

@@ +515,5 @@
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +    CodeLabel()
> +      : patchAsRawPointer_(true)
> +    { }
> +    explicit CodeLabel(bool patchAsRawPointer)

Couldn't this just have '= true' as a default value, so we won't need the constructor above?
Priority: -- → P5
Comment on attachment 8944683 [details] [diff] [review]
bug1432446.patch

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

OK, with corrections as requested in comments and below.  I have not checked the MIPS code at all.  

We do not want a bunch of #ifdef MIPS in shared files (CodeGenerator.cpp and Assembler-shared.h in this case; not too bad but really not desired).  Instead I think you should follow the pattern for JS_SMALL_BRANCH, in Assembler-shared.h, and define JS_CODELABEL_HAS_RAW_POINTER under #ifdef MIPSXX at the top of Assembler-shared.h, to control globally whether code gets to assume that we have this datum.

Also in Assembler-shared.h I would like you to rewrite the code slightly by narrowing the ifdefs, so that the constructors and patchAsRawPointer have single definitions and there are narrow ifdefs around the bodies and initializer lists.  This is so that it is clear that there is a single platform-independent interface to CodeLabel.  Ifdefs in this file are not really welcome, so let's make the best of it.

::: js/src/jit/arm64/Assembler-arm64.h
@@ +249,5 @@
>  
>      void processCodeLabels(uint8_t* rawCode) {
>          for (size_t i = 0; i < codeLabels_.length(); i++) {
>              CodeLabel label = codeLabels_[i];
> +            Bind(rawCode, *label.patchAt(), *label.target(), label.patchAtIsRawPointer());

I guess "patchAsRawPointer()".
Attachment #8944683 - Flags: review?(lhansen) → review+
Attached patch bug1432446_2.patch (obsolete) — Splinter Review
New version. Less ifdefy.
Attachment #8944683 - Attachment is obsolete: true
Attachment #8954090 - Flags: review?(lhansen)
Comment on attachment 8954090 [details] [diff] [review]
bug1432446_2.patch

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

OK.  This looks like useful cleanup of the CodeLabel machinery to me, and the changes are themselves nice and clean.  (Didn't look much at the MIPS changes though.)

::: js/src/jit/shared/Assembler-shared.h
@@ +556,5 @@
> +    };
> +private:
> +    LinkMode linkMode_ = Uninitialized;
> +
> +#endif

Nit: probably swap the endif line with the blank line above it.
Attachment #8954090 - Flags: review?(lhansen) → review+
Attached patch bug1432446_3.patch (obsolete) — Splinter Review
Attachment #8954090 - Attachment is obsolete: true
Attachment #8954302 - Flags: review+
Keywords: checkin-needed
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e283310058ba
[MIPS] Reduce size of switch table entries to a single pointer; r=lth
Keywords: checkin-needed
Ugh, sorry about that. This new version should build on x86_32. Can you tray again?
Attachment #8954302 - Attachment is obsolete: true
Flags: needinfo?(dragan.mladjenovic)
Attachment #8954662 - Flags: review+
Keywords: checkin-needed
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6729b7f0333
[MIPS] Reduce size of switch table entries to a single pointer; r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f6729b7f0333
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.