Bug 98856 – Add moveDoubleToInts and moveIntsToDouble to MacroAssemblerMIPS

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

http://trac.webkit.org/changeset/130839 broke the Qt MIPS
build, because 2 new functions introduced in it:
- moveDoubleToInts
- moveIntsToDouble

But their implementation in MacroAssemblerMIPS is still missing.

[Peter Gal]

Well I'm not a MIPS expert, but I was thinking doing something like this:

moveIntsToDouble(src_t1, src_t2, dst_fp1):
  mtc1   src_t1, dst_fp1
  mthc1  src_t2, dst_fp1

moveDoubleToInts(src_fp1, dst_t1, dst_22):
  mfc1   dst_t1, src_fp1
  mfhc1  dst_t2, src_fp1



This should work (I think at least) if the fp registers are 64bit wide. Am I right?

[Peter Gal]

Created attachment 167980 [details]
speculative fix

Well... I did something, not sure if it is correct. The MIPS gurus will decide.

[Balazs Kilvady]

Patch works but there are other missing MIPS functions I've worked on. I would like to add a full patch which contains more functions to make master branch compilable on MIPS to this bug if that is ok.

[Balazs Kilvady]

Created attachment 167989 [details]
Many missing MIPS functions

Includes Peter Gal's patch.

[WebKit Review Bot]

Attachment 167989 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/assembler/MIPSAssembler.h:376:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MIPSAssembler.h:411:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:609:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:825:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1788:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1877:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1928:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1991:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2018:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2027:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2028:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2029:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 12 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Gal]

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

>> Source/JavaScriptCore/assembler/MIPSAssembler.h:376
>> +        emitInst(0x80000000 | (rt << OP_SH_RT) | (rs << OP_SH_RS)
>> +                 | (offset & 0xffff));
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Why break these lines at all?

[Balazs Kilvady]

I tried to make the smallest diffs possible and many of those kind of lines were accepted before and personally I prefer shorter lines but I fix these files to pass the coding-style script.

[Balazs Kilvady]

Created attachment 168000 [details]
Many missing MIPS functions.

Hopefully style-fixed.

[Chao-ying Fu]

(In reply to comment #1)
> Well I'm not a MIPS expert, but I was thinking doing something like this:
> 
> moveIntsToDouble(src_t1, src_t2, dst_fp1):
>   mtc1   src_t1, dst_fp1
>   mthc1  src_t2, dst_fp1
> 
> moveDoubleToInts(src_fp1, dst_t1, dst_22):
>   mfc1   dst_t1, src_fp1
>   mfhc1  dst_t2, src_fp1
> 
> 
> 
> This should work (I think at least) if the fp registers are 64bit wide. Am I right?

Yes, but this works only for mips32r2 with -mfp64 compilation.
Note that -mfp64 is not supported under MIPS Linux O32 ABI (currently).
All double values occupy two fp registers (eg, $f0 and $f1).

We need a test to guard code.
Ex: (from in MacroAssemblerMIPS.h)

#if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
        m_assembler.mtc1(MIPSRegisters::zero, scratch);
        m_assembler.mthc1(MIPSRegisters::zero, scratch);
#else
        m_assembler.mtc1(MIPSRegisters::zero, scratch);
        m_assembler.mtc1(MIPSRegisters::zero, FPRegisterID(scratch + 1));
#endif

  Thanks!

[Balazs Kilvady]

(In reply to comment #9)
> (In reply to comment #1)
> > Well I'm not a MIPS expert, but I was thinking doing something like this:
> > 
> > moveIntsToDouble(src_t1, src_t2, dst_fp1):
> >   mtc1   src_t1, dst_fp1
> >   mthc1  src_t2, dst_fp1
> > 
> > moveDoubleToInts(src_fp1, dst_t1, dst_22):
> >   mfc1   dst_t1, src_fp1
> >   mfhc1  dst_t2, src_fp1
> > 
> > 
> > 
> > This should work (I think at least) if the fp registers are 64bit wide. Am I right?
> 
> Yes, but this works only for mips32r2 with -mfp64 compilation.
> Note that -mfp64 is not supported under MIPS Linux O32 ABI (currently).
> All double values occupy two fp registers (eg, $f0 and $f1).
> 
> We need a test to guard code.
> Ex: (from in MacroAssemblerMIPS.h)
> 
> #if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
>         m_assembler.mtc1(MIPSRegisters::zero, scratch);
>         m_assembler.mthc1(MIPSRegisters::zero, scratch);
> #else
>         m_assembler.mtc1(MIPSRegisters::zero, scratch);
>         m_assembler.mtc1(MIPSRegisters::zero, FPRegisterID(scratch + 1));
> #endif
> 
>   Thanks!

Thank you for the note, but that was a pseudo code and bot Peter Gal's patch and my patch-set contains this kind of #ifdef guards:
 void moveDoubleToInts(FPRegisterID src, RegisterID dest1, RegisterID dest2)
  {
 #if WTF_MIPS_ISA_REV(2) && WTF_MIPS_FP64
      m_assembler.mfc1(dest1, src);
      m_assembler.mfhc1(dest2, src);
 #else
     m_assembler.mfc1(dest1, src);
     m_assembler.mfc1(dest2, FPRegisterID(src + 1));
 #endif
 }

And master with my patch-set successfully compiled for MIPS and the resulted jsc passed the js tests (expect a pending regression which existed before the compilation problem but not with experimental dfg & llint implementation nor in debug compiled jsc).

[Balazs Kilvady]

Created attachment 168431 [details]
Minor code fix, style fixes, rebased

[WebKit Review Bot]

Attachment 168431 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kilvady]

Created attachment 168436 [details]
Many missing MIPS functions and style fixes.

Another style fix

[Csaba Osztrogonác]

Ping for review? (The Qt MIPS build is still broken because of this bug.)

[Oliver Hunt]

Comment on attachment 168436 [details]
Many missing MIPS functions and style fixes.

Please remove the unrelated style changes from the functionality changes.

[Balazs Kilvady]

(In reply to comment #15)
> (From update of attachment 168436 [details])
> Please remove the unrelated style changes from the functionality changes.
Those style changes was necessary to pass style checking (otherwise style checking said: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2018:  Weird number of spaces at line-start.) So I could have style-fixed only new functions but I wanted to make it consistent. Wouldn't it be reviewed/accepted this way?

[Oliver Hunt]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 168436 [details] [details])
> > Please remove the unrelated style changes from the functionality changes.
> Those style changes was necessary to pass style checking (otherwise style checking said: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2018:  Weird number of spaces at line-start.) So I could have style-fixed only new functions but I wanted to make it consistent. Wouldn't it be reviewed/accepted this way?

We don't make large (and unrelated) formatting/style changes at the same time as making a functionality change.  The formatting changes obscure the actual content of the fix.

You can do the formatting fix before or after, but not during the functionality fix.

[Balazs Kilvady]

Created attachment 168785 [details]
Many missing MIPS functions.

As Oliver Hunt suggested at review of patch #168436, this is the separated functional part of that patch. This patch should be applied after patch #168774 of bug #99359.

[Filip Pizlo]

Comment on attachment 168785 [details]
Many missing MIPS functions.

You should rebase this patch.

[Balazs Kilvady]

Created attachment 168883 [details]
Many missing MIPS functions, rebased

Previous commit likely failed because of ChangeLog diff which pended on bug 99359. Let's try with diff of a modified ChangeLog.

[Balazs Kilvady]

Comment on attachment 168883 [details]
Many missing MIPS functions, rebased

Seems it wont work without accepting 99359.

[Peter Gal]

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

> Source/JavaScriptCore/assembler/MIPSAssembler.h:525
> +    void mfhc1(RegisterID rt, FPRegisterID fs)

This function is never used.

[Csaba Osztrogonác]

(In reply to comment #21)
> (From update of attachment 168883 [details])
> Seems it wont work without accepting 99359.

Style fix landed in http://trac.webkit.org/changeset/131427

[Balazs Kilvady]

Created attachment 168894 [details]
Many missing MIPS functions, rebased #2

Next try.

[Balazs Kilvady]

Comment on attachment 168894 [details]
Many missing MIPS functions, rebased #2

void mfhc1(RegisterID rt, FPRegisterID fs) removed, thanks to Peter Gal.

[Peter Gal]

(In reply to comment #25)
> (From update of attachment 168894 [details])
> void mfhc1(RegisterID rt, FPRegisterID fs) removed, thanks to Peter Gal.

Just curious: should we use it in the vmov? or the #ifdef guards were wrong? or something else?

[Balazs Kilvady]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 168894 [details] [details])
> > void mfhc1(RegisterID rt, FPRegisterID fs) removed, thanks to Peter Gal.
> 
> Just curious: should we use it in the vmov? or the #ifdef guards were wrong? or something else?
Using vmov hides it from higher level (if we want #ifdefs later). With or without mfhc1 the operation will be implemented with two instructions so it seems no performance gain from mfhc1 just looks better then the R1 solution. If there is any reason for using it then I will add it to vmov.

[Peter Gal]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (From update of attachment 168894 [details] [details] [details])
> > > void mfhc1(RegisterID rt, FPRegisterID fs) removed, thanks to Peter Gal.
> > 
> > Just curious: should we use it in the vmov? or the #ifdef guards were wrong? or something else?
> Using vmov hides it from higher level (if we want #ifdefs later). With or without mfhc1 the operation will be implemented with two instructions so it seems no performance gain from mfhc1 just looks better then the R1 solution. If there is any reason for using it then I will add it to vmov.

Okay. BTW: why did you removed the r? flag?

[Balazs Kilvady]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > (From update of attachment 168894 [details] [details] [details] [details])
> > > > void mfhc1(RegisterID rt, FPRegisterID fs) removed, thanks to Peter Gal.
> > > 
> > > Just curious: should we use it in the vmov? or the #ifdef guards were wrong? or something else?
> > Using vmov hides it from higher level (if we want #ifdefs later). With or without mfhc1 the operation will be implemented with two instructions so it seems no performance gain from mfhc1 just looks better then the R1 solution. If there is any reason for using it then I will add it to vmov.
> 
> Okay. BTW: why did you removed the r? flag?
Mistake. If I choose + from the r list without name then Oliver or previous reviewer remains? I will try. I have to resend this patch so many times.

[Peter Gal]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > (In reply to comment #26)
> > > > (In reply to comment #25)
> > > > > (From update of attachment 168894 [details] [details] [details] [details] [details])
> > > > > void mfhc1(RegisterID rt, FPRegisterID fs) removed, thanks to Peter Gal.
> > > > 
> > > > Just curious: should we use it in the vmov? or the #ifdef guards were wrong? or something else?
> > > Using vmov hides it from higher level (if we want #ifdefs later). With or without mfhc1 the operation will be implemented with two instructions so it seems no performance gain from mfhc1 just looks better then the R1 solution. If there is any reason for using it then I will add it to vmov.
> > 
> > Okay. BTW: why did you removed the r? flag?
> Mistake. If I choose + from the r list without name then Oliver or previous reviewer remains? I will try. I have to resend this patch so many times.

Well.... if you can only set the r? flag. and a reviewer fill set it to r+. additional set the cq? flag so when a reviewer sets it to cq+ it'll be committed in time automatically. Also you don't need to re-upload it so many times, if you have want to edit the flags. (only if you wan to run the ews you should reupload, but in this case it is irrelevant)

[Balazs Kilvady]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > (In reply to comment #27)
> > > > (In reply to comment #26)
> > > > > (In reply to comment #25)
> > > > > > (From update of attachment 168894 [details] [details] [details] [details] [details] [details])
> > > > > > void mfhc1(RegisterID rt, FPRegisterID fs) removed, thanks to Peter Gal.
> > > > > 
> > > > > Just curious: should we use it in the vmov? or the #ifdef guards were wrong? or something else?
> > > > Using vmov hides it from higher level (if we want #ifdefs later). With or without mfhc1 the operation will be implemented with two instructions so it seems no performance gain from mfhc1 just looks better then the R1 solution. If there is any reason for using it then I will add it to vmov.
> > > 
> > > Okay. BTW: why did you removed the r? flag?
> > Mistake. If I choose + from the r list without name then Oliver or previous reviewer remains? I will try. I have to resend this patch so many times.
> 
> Well.... if you can only set the r? flag. and a reviewer fill set it to r+. additional set the cq? flag so when a reviewer sets it to cq+ it'll be committed in time automatically. Also you don't need to re-upload it so many times, if you have want to edit the flags. (only if you wan to run the ews you should reupload, but in this case it is irrelevant)
Thanks. But my diffs/patches wasn't applied as all of them was diff-ed to pendig or not pending versions of master branch as I was in a hurry. I try it again, now my fetched origin/master seems to be updated.

[Balazs Kilvady]

Created attachment 168910 [details]
Many missing MIPS functions, rebased #3

[WebKit Review Bot]

Comment on attachment 168910 [details]
Many missing MIPS functions, rebased #3

Clearing flags on attachment: 168910

Committed r131475: <http://trac.webkit.org/changeset/131475>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.