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

[bugzilla-daemon at webkit.org]

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

Attachment 339058: patch

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




--- Comment #3 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 339058
  --> https://bugs.webkit.org/attachment.cgi?id=339058
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.

> 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.