[Webkit-unassigned] [Bug 175447] Implement MacroAssembler::probe support on MIPS.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=175447

--- Comment #9 from Guillaume Emont <guijemont at igalia.com> ---
Comment on attachment 327416
  --> https://bugs.webkit.org/attachment.cgi?id=327416
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171122/ef0caae6/attachment-0001.html>