[webkit-reviews] review denied: [Bug 226937] Add Air opcode sub32/64(Reg, Imm, Reg) form for ARM64 and select this instruction in Air : [Attachment 431300] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Yijia Huang <yijia_huang at apple.com>'s
request for review:
Bug 226937: Add Air opcode sub32/64(Reg, Imm, Reg) form for ARM64 and select
this instruction in Air
https://bugs.webkit.org/show_bug.cgi?id=226937

Attachment 431300: Patch

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




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

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

R- but only because part of your change removes something that we don't want to
remove.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:-636
> -		   // Turn this: Sub(value, constant)
> -		   // Into this: Add(value, -constant)
> -		   if (Value* negatedConstant =
m_value->child(1)->negConstant(m_proc)) {
> -		       m_insertionSet.insertValue(m_index, negatedConstant);
> -		       replaceWithNew<Value>(
> -			   Add, m_value->origin(), m_value->child(0),
negatedConstant);
> -		       break;
> -		   }
> -		   

I don't think you want to remove this.

I get that this strength reduction makes it harder to test your change.  But,
this strength reduction is necessary so that other things work:

- It's important for strength reduction to bring programs into a canonical
form.  That is, to reduce the likelihood that two programs that do the same
thing have different IR.  Canonicalization makes it easier to then remove
redundant code.  For example: say that the program first did "Add(x, -42)" and
then did "Sub(x, 42)".	So, the two pieces of code do not seem redundant, so
common subexpression elimination (CSE) won't remove the second one.  This
canonicalization rule that you removed makes it so they will both say "Add(x,
-42)", so then CSE can remove the second one.

– Address expressions on both x86 and arm64 are matched by looking for
"Add(something, constant)".  So, it's important for other parts of the
instruction selector to see "Add(something, constant)" rather than
"Sub(something, constant)".

I would just revert this part of your change.  If you want to make sure you're
testing the instruction you added, you can write a testb3 test that skips
reduceStrength.  I'm not sure that's worth it, though.	I would just land your
change with this part reverted so you can move onto more interesting things.