Bug 175447 – Implement MacroAssembler::probe support on MIPS.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This is needed in order to enable the DFG.

[Mark Lam]

What needs to be done:
1. Copy the implementation of ctiMasmProbeTrampoline in MacroAssemblerARM.cpp into a MacroAssemblerMIPS.cpp file.  Alternatively, you may choose from MacroAssemblerARMv7.cpp, MacroAssemblerARM64.cpp, or MacroAssemblerX86Common.cpp if those are easier to work with.
2. Make sure that the COMPILE_ASSERT and static_asserts are in effect.
   This is needed to ensure that the asm code will populate the ProbeContext the way that C++ code expect it to.
3. Change the implementation to MacroAssembler::probe() into the MIPS equivalent.
4. Change the inline asm instructions in ctiMasmProbeTrampoline into the MIPS equivalent.
5. Change the part for calling a C function in ctiMasmProbeTrampoline to follow MIPS calling convention.
6. Run the testmasm test to validate the fix works before committing.

[Guillaume Emont]

I have started work on an implementation.

[Stanislav Ocovaj]

Created attachment 327414 [details]
MIPS implementation of MacroAssembler::probe()

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:56:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:57:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:58:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:59:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:60:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:61:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:62:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:63:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:64:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:65:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:96:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:97:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:98:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:99:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:100:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:101:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:102:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:103:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:104:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:105:  Extra space after (  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:504:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:395:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:424:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:501:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:538:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/testmasm.cpp:695:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 26 in 9 files


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

[Stanislav Ocovaj]

Created attachment 327416 [details]
MIPS implementation of MacroAssembler::probe()

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:504:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


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

[Guillaume Emont]

Comment on attachment 327416 [details]
MIPS implementation of MacroAssembler::probe()

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

Thanks for the hard work, I'm happy to see an implementation, though I was hoping to get mine out but you beat me to it ;). I ran testmasm with your patch on a ci20 board and everything seems to pass. Did you run tests with the patch? I just launched them on my board, but results will have to wait until tomorrow. This is an informal review as I am not a reviewer. If the tests don't show anything broken by the patch, I am personally happy with it provided you address the few changes I suggest and answer the few questions on which I have doubts.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:91
> +#define PROBE_CPU_PC_OFFSET (PROBE_FIRST_SPREG_OFFSET + (32 * GPREG_SIZE))

How about saving FCSR too? It would fit well at PROBE_FIRST_SPREG_OFFSET + (33 * GPREG_SIZE) which is empty.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:95
> +#define FPREG_SIZE 8

Since we save the fp registers as doubles, we indeed need to save 8 bytes per *even* register. Which means all the space reserved for odd registers bellow is wasted. I suggest only saving space for even registers:

#define PROBE_CPU_F0_OFFSET (PROBE_FIRST_FPREG_OFFSET + (0 * FPREG_SIZE))
#define PROBE_CPU_F2_OFFSET (PROBE_FIRST_FPREG_OFFSET + (1 * FPREG_SIZE))
#define PROBE_CPU_F4_OFFSET (PROBE_FIRST_FPREG_OFFSET + (2 * FPREG_SIZE))
etc...

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:132
> +#define PROBE_SIZE_PLUS_EXTRAS              (PROBE_SIZE + (2 * PTR_SIZE))

A comment would be welcome exlpaining what the extras are. Looks like it's SAVED_PROBE_RETURN_PC + something else. What something else? Or is it padding for alignment?

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:265
> +static_assert(!(sizeof(RARestorationRecord) & 0x7), "LRRestorationRecord must be 8-byte aligned");

I'm confused. Why does it need to be 8-byte aligned? IIRC, we need the $sp to point to an 8-aligned address at the time of a function call, but I don't see why we need the alignment here (though I might be forgetting about something). Also, I think you mean RARestorationRecord not LRRestorationRecord.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:339
> +    "addiu     $ra, $ra, 2*4" "\n" // The PC after the probe is at 2 instructions past the return point.

I think we should use defines here. E.g. "addiu $ra, $ra, PROBE_INSTRUCTIONS_AFTER_CALL * PTR_SIZE"
And maybe also add an assert (or at least a comment) at the end of MacroAssembler::probe() to make sure that PROBE_INSTRUCTIONS_AFTER_CALL is in sync with the number of instructions after the trampoline call.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:410
> +    // out of the Probe::State before returning, except for zero, k0 and k1.

I am wondering: is it safe to restore $at too?

[Stanislav Ocovaj]

Comment on attachment 327416 [details]
MIPS implementation of MacroAssembler::probe()

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

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:91
>> +#define PROBE_CPU_PC_OFFSET (PROBE_FIRST_SPREG_OFFSET + (32 * GPREG_SIZE))
> 
> How about saving FCSR too? It would fit well at PROBE_FIRST_SPREG_OFFSET + (33 * GPREG_SIZE) which is empty.

I will add saving/restoring of FCSR and other control registers

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:95
>> +#define FPREG_SIZE 8
> 
> Since we save the fp registers as doubles, we indeed need to save 8 bytes per *even* register. Which means all the space reserved for odd registers bellow is wasted. I suggest only saving space for even registers:
> 
> #define PROBE_CPU_F0_OFFSET (PROBE_FIRST_FPREG_OFFSET + (0 * FPREG_SIZE))
> #define PROBE_CPU_F2_OFFSET (PROBE_FIRST_FPREG_OFFSET + (1 * FPREG_SIZE))
> #define PROBE_CPU_F4_OFFSET (PROBE_FIRST_FPREG_OFFSET + (2 * FPREG_SIZE))
> etc...

The problem here is that these offsets must match register ID's defined by the FPRegisterID enum. To save the space in the context we would have to modify those enums, but then they would not match real FPregister ID's as they are used in MIPS assembly, so I suggest that we leave this as it is.

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:132
>> +#define PROBE_SIZE_PLUS_EXTRAS              (PROBE_SIZE + (2 * PTR_SIZE))
> 
> A comment would be welcome exlpaining what the extras are. Looks like it's SAVED_PROBE_RETURN_PC + something else. What something else? Or is it padding for alignment?

The extras are SAVED_PROBE_RETURN_PC plus the padding

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:265
>> +static_assert(!(sizeof(RARestorationRecord) & 0x7), "LRRestorationRecord must be 8-byte aligned");
> 
> I'm confused. Why does it need to be 8-byte aligned? IIRC, we need the $sp to point to an 8-aligned address at the time of a function call, but I don't see why we need the alignment here (though I might be forgetting about something). Also, I think you mean RARestorationRecord not LRRestorationRecord.

MIPS calling convention requires that the stack is 8-byte aligned at all times, not only at the time of a function call. This is because the code might be interrupted at any time by a signal, and signal handler might need to push a double value to the stack

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:339
>> +    "addiu     $ra, $ra, 2*4" "\n" // The PC after the probe is at 2 instructions past the return point.
> 
> I think we should use defines here. E.g. "addiu $ra, $ra, PROBE_INSTRUCTIONS_AFTER_CALL * PTR_SIZE"
> And maybe also add an assert (or at least a comment) at the end of MacroAssembler::probe() to make sure that PROBE_INSTRUCTIONS_AFTER_CALL is in sync with the number of instructions after the trampoline call.

I will change it to 2 * PTR_SIZE. This is how it's done on ARM64. I don't see a simple way to check if this is in sync with the actual number of instructions

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:410
>> +    // out of the Probe::State before returning, except for zero, k0 and k1.
> 
> I am wondering: is it safe to restore $at too?

I don't see why it wouldn't be safe, as long as the probe's user knows what he's doing. The real question here is whether it is *possible* to save/restore some registers. Zero obviously cannot be set to any value, and k0 and k1 may be modified by the kernel at any time, so that's the main reason why these registers are excluded.

[Guillaume Emont]

Comment on attachment 327416 [details]
MIPS implementation of MacroAssembler::probe()

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

I ran the tests with your patch, and I only got 1 test failure, which seems to be an out of memory issue, which is a huge progress compared to the ~156 we currently have upstream.

>>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:95
>>> +#define FPREG_SIZE 8
>> 
>> Since we save the fp registers as doubles, we indeed need to save 8 bytes per *even* register. Which means all the space reserved for odd registers bellow is wasted. I suggest only saving space for even registers:
>> 
>> #define PROBE_CPU_F0_OFFSET (PROBE_FIRST_FPREG_OFFSET + (0 * FPREG_SIZE))
>> #define PROBE_CPU_F2_OFFSET (PROBE_FIRST_FPREG_OFFSET + (1 * FPREG_SIZE))
>> #define PROBE_CPU_F4_OFFSET (PROBE_FIRST_FPREG_OFFSET + (2 * FPREG_SIZE))
>> etc...
> 
> The problem here is that these offsets must match register ID's defined by the FPRegisterID enum. To save the space in the context we would have to modify those enums, but then they would not match real FPregister ID's as they are used in MIPS assembly, so I suggest that we leave this as it is.

What if we define FPREG_SIZE as 4?
Then we should still be able to save/restore with ldc1 on the even registers, which would just be an optimization internal to the probe, but then we would consider the individual single float registers from the outside?

>>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:132
>>> +#define PROBE_SIZE_PLUS_EXTRAS              (PROBE_SIZE + (2 * PTR_SIZE))
>> 
>> A comment would be welcome exlpaining what the extras are. Looks like it's SAVED_PROBE_RETURN_PC + something else. What something else? Or is it padding for alignment?
> 
> The extras are SAVED_PROBE_RETURN_PC plus the padding

I think a comment explaining that would be welcome.

>>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:339
>>> +    "addiu     $ra, $ra, 2*4" "\n" // The PC after the probe is at 2 instructions past the return point.
>> 
>> I think we should use defines here. E.g. "addiu $ra, $ra, PROBE_INSTRUCTIONS_AFTER_CALL * PTR_SIZE"
>> And maybe also add an assert (or at least a comment) at the end of MacroAssembler::probe() to make sure that PROBE_INSTRUCTIONS_AFTER_CALL is in sync with the number of instructions after the trampoline call.
> 
> I will change it to 2 * PTR_SIZE. This is how it's done on ARM64. I don't see a simple way to check if this is in sync with the actual number of instructions

What I am proposing is something more manual done in ::probe like:

    m_assembler.jalr(s0);
    m_assembler.nop();
    load32(Address(sp, offsetof(RARestorationRecord, ra)), ra);
    add32(TrustedImm32(sizeof(RARestorationRecord)), sp);
    static_assert(PROBE_INSTRUCTIONS_AFTER_CALL == 2); // this needs to be the exact number of instructions after jalr and nop.

Or maybe an assert is overkill and a simple comment would work:
    // if you change this, make sure it's in sync with the definition of PROBE_INSTRUCTIONS_AFTER_CALL

[Stanislav Ocovaj]

Comment on attachment 327416 [details]
MIPS implementation of MacroAssembler::probe()

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

>>>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:95
>>>> +#define FPREG_SIZE 8
>>> 
>>> Since we save the fp registers as doubles, we indeed need to save 8 bytes per *even* register. Which means all the space reserved for odd registers bellow is wasted. I suggest only saving space for even registers:
>>> 
>>> #define PROBE_CPU_F0_OFFSET (PROBE_FIRST_FPREG_OFFSET + (0 * FPREG_SIZE))
>>> #define PROBE_CPU_F2_OFFSET (PROBE_FIRST_FPREG_OFFSET + (1 * FPREG_SIZE))
>>> #define PROBE_CPU_F4_OFFSET (PROBE_FIRST_FPREG_OFFSET + (2 * FPREG_SIZE))
>>> etc...
>> 
>> The problem here is that these offsets must match register ID's defined by the FPRegisterID enum. To save the space in the context we would have to modify those enums, but then they would not match real FPregister ID's as they are used in MIPS assembly, so I suggest that we leave this as it is.
> 
> What if we define FPREG_SIZE as 4?
> Then we should still be able to save/restore with ldc1 on the even registers, which would just be an optimization internal to the probe, but then we would consider the individual single float registers from the outside?

We can't do that, either, because the Probe::State::cpu.fprs array is declared as double.

>>>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:132
>>>> +#define PROBE_SIZE_PLUS_EXTRAS              (PROBE_SIZE + (2 * PTR_SIZE))
>>> 
>>> A comment would be welcome exlpaining what the extras are. Looks like it's SAVED_PROBE_RETURN_PC + something else. What something else? Or is it padding for alignment?
>> 
>> The extras are SAVED_PROBE_RETURN_PC plus the padding
> 
> I think a comment explaining that would be welcome.

OK, I'll add a comment

>>>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:339
>>>> +    "addiu     $ra, $ra, 2*4" "\n" // The PC after the probe is at 2 instructions past the return point.
>>> 
>>> I think we should use defines here. E.g. "addiu $ra, $ra, PROBE_INSTRUCTIONS_AFTER_CALL * PTR_SIZE"
>>> And maybe also add an assert (or at least a comment) at the end of MacroAssembler::probe() to make sure that PROBE_INSTRUCTIONS_AFTER_CALL is in sync with the number of instructions after the trampoline call.
>> 
>> I will change it to 2 * PTR_SIZE. This is how it's done on ARM64. I don't see a simple way to check if this is in sync with the actual number of instructions
> 
> What I am proposing is something more manual done in ::probe like:
> 
>     m_assembler.jalr(s0);
>     m_assembler.nop();
>     load32(Address(sp, offsetof(RARestorationRecord, ra)), ra);
>     add32(TrustedImm32(sizeof(RARestorationRecord)), sp);
>     static_assert(PROBE_INSTRUCTIONS_AFTER_CALL == 2); // this needs to be the exact number of instructions after jalr and nop.
> 
> Or maybe an assert is overkill and a simple comment would work:
>     // if you change this, make sure it's in sync with the definition of PROBE_INSTRUCTIONS_AFTER_CALL

I'll add a comment here

[Stanislav Ocovaj]

Created attachment 327502 [details]
MIPS implementation of MacroAssembler::probe()

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerMIPS.cpp:551:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


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

[Guillaume Emont]

I'm happy with the shape of the patch. Currently running tests with it which will take a few more hours, but I'm hopeful it won't introduce regressions as the previous version of the patch did not. Somebody else will have to give an r+ though, as I am not a reviewer.

[Guillaume Emont]

(In reply to Guillaume Emont from comment #13)
> I'm happy with the shape of the patch. Currently running tests with it which
> will take a few more hours, but I'm hopeful it won't introduce regressions
> as the previous version of the patch did not. Somebody else will have to
> give an r+ though, as I am not a reviewer.

The tests have run: only 2 failures left when running with a 1200 seconds timeout (compared to about 160 in svn in testbot), and they both seem to be related to the limited amount of memory available. (This is on a ci20 board).

[Carlos Alberto Lopez Perez]

Comment on attachment 327502 [details]
MIPS implementation of MacroAssembler::probe()

r=me based on previous review from Guillaume that has reviewed and tested it

[WebKit Commit Bot]

Comment on attachment 327502 [details]
MIPS implementation of MacroAssembler::probe()

Clearing flags on attachment: 327502

Committed r225286: <https://trac.webkit.org/changeset/225286>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35754206>

[Matt Lewis]

Reverted r225286 for reason:

The source files within this patch have been marked as executable.

Committed r225293: <https://trac.webkit.org/changeset/225293>

[Guillaume Emont]

(In reply to Matt Lewis from comment #19)
> Reverted r225286 for reason:
> 
> The source files within this patch have been marked as executable.
> 
> Committed r225293: <https://trac.webkit.org/changeset/225293>

Oh, I missed that.
Stanislav: please provide a new version of the patch that does not change permissions of existing files, and that sets the permissions of assembler/MacroAssemblerMIPS.cpp to the same as the other files (644, no executable bit set).

[Carlos Alberto Lopez Perez]

(In reply to Matt Lewis from comment #19)
> Reverted r225286 for reason:
> 
> The source files within this patch have been marked as executable.
> 
> Committed r225293: <https://trac.webkit.org/changeset/225293>

It think it would have been easier to fix the permissions instead of reverting the patch. Also it seems the revert bot is not very smart about this because Source/JavaScriptCore/ChangeLog and Source/WTF/ChangeLog continue to be executable.

I will try to fix this manually

[Carlos Alberto Lopez Perez]

Committed r225301: <https://trac.webkit.org/changeset/225301>

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #22)
> Committed r225301: <https://trac.webkit.org/changeset/225301>

I re-landed it at the same time I fixed the permissions issue.

Thanks for notifying this issue with the permissions.