Bug 123799

Summary: REGRESSION(r158315): register mixup in JIT::compileOpCall
Product: WebKit Reporter: Julien Brianceau <jbriance>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, gergely, kilvadyb, mark.lam, msaboff
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Use regT3 instead of GPRInfo::regT3 in JIT::compileOpCall none

Description Julien Brianceau 2013-11-05 04:49:41 PST 
Changeset 158315 (http://trac.webkit.org/changeset/158315) is crashing architectures where JSInterfaceJIT::regT3 is different from GPRInfo::regT3, like in mips.
Comment 1 Julien Brianceau 2013-11-05 04:54:33 PST 
Created attachment 216026 [details]
Use regT3 instead of GPRInfo::regT3 in JIT::compileOpCall
Comment 2 Mark Lam 2013-11-05 07:16:41 PST 
Comment on attachment 216026 [details]
Use regT3 instead of GPRInfo::regT3 in JIT::compileOpCall

r=me
Comment 3 WebKit Commit Bot 2013-11-05 07:32:07 PST 
Comment on attachment 216026 [details]
Use regT3 instead of GPRInfo::regT3 in JIT::compileOpCall

Clearing flags on attachment: 216026

Committed r158672: <http://trac.webkit.org/changeset/158672>
Comment 4 WebKit Commit Bot 2013-11-05 07:32:10 PST 
All reviewed patches have been landed.  Closing bug.
Comment 5 Filip Pizlo 2013-11-05 08:23:11 PST 
Why does MIPS use a different definition of regT3?

Why not fix that problem?
Comment 6 Mark Lam 2013-11-05 08:25:56 PST 
(In reply to comment #5)
> Why does MIPS use a different definition of regT3?
> 
> Why not fix that problem?

I agree that we should make the register assignments consistent.  However, MIPS isn't the only CPU port that has this issue.  For now, I r+ the change to resolve the immediate action.  Longer term, we should look into unifying the register assignments in GPRInfo.h and JSInterfaceJIT.h.
Comment 7 Filip Pizlo 2013-11-05 08:27:25 PST 
(In reply to comment #6)
> (In reply to comment #5)
> > Why does MIPS use a different definition of regT3?
> > 
> > Why not fix that problem?
> 
> I agree that we should make the register assignments consistent.  However, MIPS isn't the only CPU port that has this issue.  For now, I r+ the change to resolve the immediate action.  Longer term, we should look into unifying the register assignments in GPRInfo.h and JSInterfaceJIT.h.

Which others have this problem?
Comment 8 Julien Brianceau 2013-11-05 08:27:47 PST 
(In reply to comment #5)
> Why does MIPS use a different definition of regT3?
Good question, I don't know why MIPS backend has been designed like this.
 
> Why not fix that problem?
I can try if you think it's worth to do so.
Comment 9 Mark Lam 2013-11-05 08:36:44 PST 
(In reply to comment #7)
> Which others have this problem?

Apart from MIPS, at a glance ...

For this exact problem:

CPU(ARM64):  regT3:  x23    vs    x3

For other discrepancies between GPRInfo.h and JSInterfaceJIT.h:

CPU(X86_86): regT4:   r10     vs    edi
CPU(ARM):      regT4:   r7      vs    r8
CPU(ARM64):  regT4:   x24    vs    x4
Comment 10 Filip Pizlo 2013-11-05 08:59:15 PST 
(In reply to comment #9)
> (In reply to comment #7)
> > Which others have this problem?
> 
> Apart from MIPS, at a glance ...
> 
> For this exact problem:
> 
> CPU(ARM64):  regT3:  x23    vs    x3
> 
> For other discrepancies between GPRInfo.h and JSInterfaceJIT.h:
> 
> CPU(X86_86): regT4:   r10     vs    edi
> CPU(ARM):      regT4:   r7      vs    r8
> CPU(ARM64):  regT4:   x24    vs    x4

We should fix that by getting rid of the regT* definitions in JSInterfaceJIT.h

If some backend can't handle it then that's a separate problem.
Comment 11 Mark Lam 2013-11-05 09:05:15 PST 
(In reply to comment #10)
> We should fix that by getting rid of the regT* definitions in JSInterfaceJIT.h

I'll work on it in https://bugs.webkit.org/show_bug.cgi?id=123806.
Comment 12 Julien Brianceau 2013-11-05 09:09:26 PST 
As discussed with Mark on IRC, I'll submit a patch soon to fix MIPS backend in https://bugs.webkit.org/show_bug.cgi?id=123807