[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 #7 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

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?

-- 
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/71799c4b/attachment.html>