[webkit-reviews] review granted: [Bug 227509] Add Air opcode add/sub-and-shift for ARM64 and select this instruction in Air : [Attachment 433016] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Yijia Huang
<yijia_huang at apple.com>'s request for review:
Bug 227509: Add Air opcode add/sub-and-shift for ARM64 and select this
instruction in Air
https://bugs.webkit.org/show_bug.cgi?id=227509

Attachment 433016: Patch

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




--- Comment #8 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 433016
  --> https://bugs.webkit.org/attachment.cgi?id=433016
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433016&action=review

This is really cool!  I don't feel too strongly about it, but I'd recommend not
doing canBeInternal/commitInternal for this one.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2616
> +		   if (!canBeInternal(right) || !imm(right->child(1)) ||
right->child(1)->asInt() < 0)

I wouldn't do canBeInternal for this one.  I'd assume that an add-with-shift
instruction is no more expensive than an add instruction.  So, if we have a
program that does a shift and then uses the result of that shift both directly
and for an add, then emitting the add as an add-with-shift instruction is fine.
 In fact, it might even be more efficient, since it removes a dependency.  If
you have a shift and then an add that depends on the shift, then the add can't
run until the shift finishes.  If you have a shift and then an add-with-shift
that depends on the same input as the shift, then both the shift and the
add-with-shift can execute in parallel.