Bug 125168 – [MIPS] Wrong register usage in LLInt op_catch.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Balazs Kilvady]

Earlier trampoline called lint_op_catch via register ra (return-from-call register) while new exception handler calls the catch op via t9 reg. This change caused 13 regressions in mozilla test.

[Balazs Kilvady]

Created attachment 218311 [details]
proposed patch.

[Filip Pizlo]

Comment on attachment 218311 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=218311&action=review

> Source/JavaScriptCore/offlineasm/mips.rb:-535
> -            if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name)
> +            if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) || /_llint_op_catch/.match(node.name)
>                  # Functions called from trampoline/JIT codes.
>                  myList << Instruction.new(node.codeOrigin, "pichdr", [])
> -            elsif /_llint_op_catch/.match(node.name)
> -                # Exception cactcher entry point function.
> -                myList << Instruction.new(node.codeOrigin, "pichdrra", [])

This is atrocious.  Are you seriously matching label names?

[Balazs Kilvady]

(In reply to comment #2)
> (From update of attachment 218311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218311&action=review
> 
> > Source/JavaScriptCore/offlineasm/mips.rb:-535
> > -            if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name)
> > +            if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) || /_llint_op_catch/.match(node.name)
> >                  # Functions called from trampoline/JIT codes.
> >                  myList << Instruction.new(node.codeOrigin, "pichdr", [])
> > -            elsif /_llint_op_catch/.match(node.name)
> > -                # Exception cactcher entry point function.
> > -                myList << Instruction.new(node.codeOrigin, "pichdrra", [])
> 
> This is atrocious.  Are you seriously matching label names?

Yes. On MIPS for PIC compatibility a special header (pichdr) needed. I didn't find a better way to add PIC header only to the "entry point" functions/ops.

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 218311 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=218311&action=review
> > 
> > > Source/JavaScriptCore/offlineasm/mips.rb:-535
> > > -            if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name)
> > > +            if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) || /_llint_op_catch/.match(node.name)
> > >                  # Functions called from trampoline/JIT codes.
> > >                  myList << Instruction.new(node.codeOrigin, "pichdr", [])
> > > -            elsif /_llint_op_catch/.match(node.name)
> > > -                # Exception cactcher entry point function.
> > > -                myList << Instruction.new(node.codeOrigin, "pichdrra", [])
> > 
> > This is atrocious.  Are you seriously matching label names?
> 
> Yes. On MIPS for PIC compatibility a special header (pichdr) needed. I didn't find a better way to add PIC header only to the "entry point" functions/ops.

This is really bad.  Please find a better way to do this before making the problem worse.

[Balazs Kilvady]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 218311 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=218311&action=review
> > > 
> > > > Source/JavaScriptCore/offlineasm/mips.rb:-535
> > > > -            if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name)
> > > > +            if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) || /_llint_op_catch/.match(node.name)
> > > >                  # Functions called from trampoline/JIT codes.
> > > >                  myList << Instruction.new(node.codeOrigin, "pichdr", [])
> > > > -            elsif /_llint_op_catch/.match(node.name)
> > > > -                # Exception cactcher entry point function.
> > > > -                myList << Instruction.new(node.codeOrigin, "pichdrra", [])
> > > 
> > > This is atrocious.  Are you seriously matching label names?
> > 
> > Yes. On MIPS for PIC compatibility a special header (pichdr) needed. I didn't find a better way to add PIC header only to the "entry point" functions/ops.
> 
> This is really bad.  Please find a better way to do this before making the problem worse.
Thank you for the review. I dislike this solution also but PIC header is 3 instructions long so I didn't want to add it to all the LLInt ops when it is not necessary. I think there is no other sign of entry point code than the node.name from offlineasm/mips.rb. I could add PIC header to all the ops (like gcc does for every function). I will check the speed penalty of that solution.

[Balazs Kilvady]

Created attachment 218585 [details]
Fixed patch. 

Added PIC header (.cpload($t9)) to all the LLInt ops. With this we can avoid filtering rutins by label name. The speed penalty is not serious by the SunSpider and v8 tests (although they aren't ideal for LLInt measuring, I think).
SunSpider: 5272.0ms -> 5295.0ms
v8: 18751.0ms -> 18893.0ms

With this patch the MIPS implementation has 0 regression in mozilla tests.

[Julien Brianceau]

Patch looks good to me: it solves all remaining crashes in layout jsc tests on my mips board.

Moreover, it removes the following warnings when compiling the LLINT:

   [100%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
   /tmp/cc9i3yBA.s: Assembler messages:
   /tmp/cc9i3yBA.s:270: Warning: No .cprestore pseudo-op used in PIC code
   /tmp/cc9i3yBA.s:300: Warning: No .cprestore pseudo-op used in PIC code
   /tmp/cc9i3yBA.s:332: Warning: No .cprestore pseudo-op used in PIC code
   /tmp/cc9i3yBA.s:375: Warning: No .cprestore pseudo-op used in PIC code
   [...]


Although there's a small speed penalty, I think your 2nd patch is better (and more clean).

[Balazs Kilvady]

(In reply to comment #7)
> Patch looks good to me: it solves all remaining crashes in layout jsc tests on my mips board.
> 
> Moreover, it removes the following warnings when compiling the LLINT:
> 
>    [100%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
>    /tmp/cc9i3yBA.s: Assembler messages:
>    /tmp/cc9i3yBA.s:270: Warning: No .cprestore pseudo-op used in PIC code
>    /tmp/cc9i3yBA.s:300: Warning: No .cprestore pseudo-op used in PIC code
>    /tmp/cc9i3yBA.s:332: Warning: No .cprestore pseudo-op used in PIC code
>    /tmp/cc9i3yBA.s:375: Warning: No .cprestore pseudo-op used in PIC code
>    [...]
> 
> 
> Although there's a small speed penalty, I think your 2nd patch is better (and more clean).

Thanks for reviewing and testing. The disappearing of the "No .cprestore" warning is a nice side effect as .cprestore is still not used in the LLInt ops. :) But anyway now the generated code seems to be more comfortable to the compiler.

[Balazs Kilvady]

r160244 introduced a conditional branch from an LLInt op to an other. This breaks the "PIC header everywhere" patch. In mozilla test causes a regression:
ecma_2/FunctionObjects/apply-001-n.js
I am working on fixing this problem.

[Balazs Kilvady]

Created attachment 218975 [details]
Fixed and rebased patch. 

Branch to other LLInt operations (to Labels, not to LocalLabels) fixed also. Rebased on r160432.

[Geoffrey Garen]

Comment on attachment 218975 [details]
Fixed and rebased patch. 

I don't really know this platform. Since all the code is isolated to it, I don't think it can harm the rest of the project, so I'll say r+.

[Julien Brianceau]

(In reply to comment #11)
> I don't really know this platform. Since all the code is isolated to it, I don't think it can harm the rest of the project, so I'll say r+.
Thanks. As discussed, I'll probably take a look at the mips port during next days, and this patch will help me to start from a clean state.

[WebKit Commit Bot]

Comment on attachment 218975 [details]
Fixed and rebased patch. 

Clearing flags on attachment: 218975

Committed r173232: <http://trac.webkit.org/changeset/173232>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.