[Webkit-unassigned] [Bug 183243] [MIPS] Optimize generated JIT code for loads/stores from/to an absolute address

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