[webkit-reviews] review requested: [Bug 185106] Macro assembler micro-optimizations for x86-64 and ARM64 : [Attachment 339149] patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has asked  for review:
Bug 185106: Macro assembler micro-optimizations for x86-64 and ARM64
https://bugs.webkit.org/show_bug.cgi?id=185106

Attachment 339149: patch

https://bugs.webkit.org/attachment.cgi?id=339149&action=review




--- Comment #6 from JF Bastien <jfbastien at apple.com> ---
Created attachment 339149

  --> https://bugs.webkit.org/attachment.cgi?id=339149&action=review

patch

Address whitespace issues.


(In reply to Filip Pizlo from comment #3)
> Comment on attachment 339058 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339058&action=review
> 
> Please remove that assertion.
> 
> > Source/JavaScriptCore/assembler/MacroAssembler.h:370
> > +	     ASSERT((imm.m_value & 31) == imm.m_value);
> 
> This is not a valid assertion. It’s fine to call shift with an immediate
> that violates this assertion.

Removed.

I find this surprising though: wouldn't that mean that we've knowingly
propagated an out-of-range value? I'd have expected us to hold it in range as
an invariant, otherwise all the code that reasons about shifts with know RHS
has to reason with a mask (not hard, but bug-prone, which this assert would
catch). In practice that's immaterial for x86 and ARM64, but on ARM32 it means
intermediate optimizations can get the wrong result because ARM32 masks the RHS
with 0xFF. That would give the wrong JS result.


> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:283
> > +	     RegisterID scratch = src != dest ? dest :
getCachedDataTempRegisterIDAndInvalidate();
> 
> How is this better?  I would revert this change unless you had some great
> evidence.

Sorry the description I have is bad: this is better because you don't need to
allow-temp. It's not an optimization, it's about correctness and ease of use.
I've tripped on that bug before, and it seems dangerous because the "can I use
temp-registers" assertion is debug-only, so code that seems right and passes
all tests can still fail at runtime by clobbering a register unexpectedly. This
change makes it easier to avoid this pitfall (pass src != dest), and less
likely that we hit a silent register clobber.

Would you like me to:

 1) Drop this entirely.
 2) Remove from this patch, put in a separate one with a better description.
 3) Keep here, and fix this patch's description.