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

Status: RESOLVED FIXED

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

Description

[Dragan Mladjenovic]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Comment 1

[Dragan Mladjenovic]

Attachment: bug1432446.patch

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.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 3

[Lars T Hansen [:lth]]

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?

Comment 4

[Lars T Hansen [:lth]]

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()".

Comment 5

[Dragan Mladjenovic]

Attachment: bug1432446_2.patch

New version. Less ifdefy.

Comment 6

[Lars T Hansen [:lth]]

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.

Comment 7

[Dragan Mladjenovic]

Attachment: bug1432446_3.patch

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e283310058ba
[MIPS] Reduce size of switch table entries to a single pointer; r=lth

Comment 9

[Andreea Pavel [:apavel]]

Backed out for build bustages at /builds/worker/workspace/build/src/js/src/jit/x86/Trampoline-x86.cpp:185

Push that caused the bustages: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e283310058ba4a0358fd3ea844d88cd66d72fd20

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164749580&repo=mozilla-inbound&lineNumber=8157

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/24c4795c13be364230a324c525de51eabea481da

Comment 10

[Dragan Mladjenovic]

Attachment: bug1432446_4.patch

Ugh, sorry about that. This new version should build on x86_32. Can you tray again?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6729b7f0333
[MIPS] Reduce size of switch table entries to a single pointer; r=lth

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f6729b7f0333