Bug 125168 - [MIPS] Wrong register usage in LLInt op_catch.
Summary: [MIPS] Wrong register usage in LLInt op_catch.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 108664
  Show dependency treegraph
 
Reported: 2013-12-03 10:53 PST by Balazs Kilvady
Modified: 2014-09-03 14:51 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch. (3.59 KB, patch)
2013-12-03 11:02 PST, Balazs Kilvady
fpizlo: review-
fpizlo: commit-queue-
Details | Formatted Diff | Diff
Fixed patch. (7.92 KB, patch)
2013-12-06 03:15 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff
Fixed and rebased patch. (9.97 KB, patch)
2013-12-11 10:48 PST, Balazs Kilvady
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kilvady 2013-12-03 10:53:12 PST
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.
Comment 1 Balazs Kilvady 2013-12-03 11:02:56 PST
Created attachment 218311 [details]
proposed patch.
Comment 2 Filip Pizlo 2013-12-03 11:04:50 PST
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?
Comment 3 Balazs Kilvady 2013-12-03 11:12:31 PST
(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.
Comment 4 Filip Pizlo 2013-12-03 11:15:17 PST
(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.
Comment 5 Balazs Kilvady 2013-12-03 11:34:02 PST
(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.
Comment 6 Balazs Kilvady 2013-12-06 03:15:49 PST
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.
Comment 7 Julien Brianceau 2013-12-06 05:26:31 PST
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).
Comment 8 Balazs Kilvady 2013-12-06 09:59:49 PST
(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.
Comment 9 Balazs Kilvady 2013-12-11 07:19:08 PST
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.
Comment 10 Balazs Kilvady 2013-12-11 10:48:18 PST
Created attachment 218975 [details]
Fixed and rebased patch. 

Branch to other LLInt operations (to Labels, not to LocalLabels) fixed also. Rebased on r160432.
Comment 11 Geoffrey Garen 2014-09-03 14:13:45 PDT
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+.
Comment 12 Julien Brianceau 2014-09-03 14:21:33 PDT
(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.
Comment 13 WebKit Commit Bot 2014-09-03 14:50:58 PDT
Comment on attachment 218975 [details]
Fixed and rebased patch. 

Clearing flags on attachment: 218975

Committed r173232: <http://trac.webkit.org/changeset/173232>
Comment 14 WebKit Commit Bot 2014-09-03 14:51:02 PDT
All reviewed patches have been landed.  Closing bug.