Bug 98856

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?

Created attachment 167980 [details] speculative fix Well... I did something, not sure if it is correct. The MIPS gurus will decide.

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.

Created attachment 167989 [details] Many missing MIPS functions Includes Peter Gal's patch.

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.

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?

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.

Created attachment 168000 [details] Many missing MIPS functions. Hopefully style-fixed.

(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!

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

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

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.

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

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

Comment on attachment 168436 [details] Many missing MIPS functions and style fixes. Please remove the unrelated style changes from the functionality changes.

(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?

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

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.

Comment on attachment 168785 [details] Many missing MIPS functions. You should rebase this patch.

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.

Comment on attachment 168883 [details] Many missing MIPS functions, rebased Seems it wont work without accepting 99359.

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.

(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

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

Comment on attachment 168894 [details] Many missing MIPS functions, rebased #2 void mfhc1(RegisterID rt, FPRegisterID fs) removed, thanks to 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?

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

(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?

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

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

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

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

Comment on attachment 168910 [details] Many missing MIPS functions, rebased #3 Clearing flags on attachment: 168910 Committed r131475: <http://trac.webkit.org/changeset/131475>

All reviewed patches have been landed. Closing bug.