[Webkit-unassigned] [Bug 183243] [MIPS] Optimize generated JIT code for loads/stores from/to an absolute address
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Mar 7 10:32:10 PST 2018
https://bugs.webkit.org/show_bug.cgi?id=183243
Adrian Perez <aperez at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |aperez at igalia.com
--- Comment #5 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 335180
--> https://bugs.webkit.org/attachment.cgi?id=335180
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=335180&action=review
Hi there! Though I am not officially a reviewer, I have been doing informal
reviews on MIPS-related patches. This patch looks good to me, and even when
shaving one instruction per load/store may not look like a lot, it probably
makes for measurable savings as there will be many load/store operations in
typical generated code (By th way: do you have any performance numbers with
and without the patch? — I am sure this is something we want to merge, I am
just being curious myself :-D).
I have just a small comment about using “uintptr_t” instead of
“uint32_t”; but it's not something that should block landing the
patch.
Thanks a lot for your contribution!
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:239
> + uint32_t adr = reinterpret_cast<uint32_t>(address.m_ptr);
Small nit: I would use “uintptr_t” here. Technically it will be
equivalent, but IMHO it makes the intention of storing a pointer
as an integer value clearer.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:242
> + if (imm.m_value >= -32768 && imm.m_value <= 32767)
I know that this statement was just moved around and was already
written this way, but maybe it would be nicer to use “INT16_MIN”
and “INT16_MAX” instead of writing the not-so-magic numbers in
the condition check.
I am fine leaving the patch as-is currently, and if we want to use
the constant names I would change that in a separate patch. At any
rate, I am just thinking out loud, and I am not sure whether this
is worth caring about — what do other reviewers think about this?
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:268
> + uint32_t adr = reinterpret_cast<uint32_t>(address.m_ptr);
Same here about using “uintptr_t”.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:453
> + uint32_t adr = reinterpret_cast<uint32_t>(dest.m_ptr);
Same here about using “uintptr_t”.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:507
> + uint32_t adr = reinterpret_cast<uint32_t>(dest.m_ptr);
Same here about using “uintptr_t”.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:693
> + uint32_t adr = reinterpret_cast<uint32_t>(address.m_ptr);
Same here about using “uintptr_t”.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:879
> + uint32_t adr = reinterpret_cast<uint32_t>(address);
Same here about using “uintptr_t”.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:945
> + uint32_t adr = reinterpret_cast<uint32_t>(address);
Same here about using “uintptr_t”.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1111
> + uint32_t adr = reinterpret_cast<uint32_t>(address);
Same here about using “uintptr_t”.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1271
> + uint32_t adr = reinterpret_cast<uint32_t>(address);
Same here about using “uintptr_t”.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1290
> + uint32_t adr = reinterpret_cast<uint32_t>(address);
Ditto.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1478
> + uint32_t adr = reinterpret_cast<uint32_t>(address);
Ditto.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1496
> + uint32_t adr = reinterpret_cast<uint32_t>(address);
Ditto.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2091
> + uint32_t adr = reinterpret_cast<uint32_t>(dest.m_ptr);
Ditto.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2714
> + uint32_t adr = reinterpret_cast<uint32_t>(address.m_value);
Ditto.
> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2870
> + uint32_t adr = reinterpret_cast<uint32_t>(address.m_value);
Ditto.
--
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/20180307/fcff4efc/attachment-0001.html>
More information about the webkit-unassigned
mailing list