[PATCH] D108139: [X86] Freeze vXi8 shl(x,1) -> add(x,x) vector fold (PR50468)

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:28729
 
-    // Simple i8 add case
-    if (Op.getOpcode() == ISD::SHL && ShiftAmt == 1)
+    // Simple i8 add case (freeze to handle add(undef, undef) case).
+    if (Op.getOpcode() == ISD::SHL && ShiftAmt == 1) {
----------------
pengfei wrote:
> craig.topper wrote:
> > pengfei wrote:
> > > Sorry, I don't quite understand the background. My question is why don't we check the oprand is a undef?
> > > The affected tests seem all reasonable values, though they seem not have regressions.
> > It’s not enough to check for undef at the time of the fold. You need to know that no future DAGCombine can make the input undef.
> > 
> > (shl X, 1) must produce an even number even if X is undef. computeKnownBits will look at the shift amount and tell downstream code that the lsb is 0 without looking at the left hand side. And the value may not be undef at the time computeKnownBits is called but could be simplified to undef later.
> > 
> > (add undef, undef) does not produce an even number. Register allocation is free to pick different registers for undef.
> > 
> > The freeze forces register allocation to use the same register.
> > 
> > 
> Nice answer! I'm clear now. Thanks Craig.
Undef reasoning is complicated, and the explanation here is great. How about enshrining it as a code comment? :)
  // R may be undef at run-time, but (shl R, 1) must be an even number (LSB must be 0).
  // (add undef, undef) however can be any value. To make this safe, we must freeze R
  // to ensure that register allocation uses the same register for an undefined value.
  // This ensures that the result will still be even and preserves the original semantics.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108139/new/

https://reviews.llvm.org/D108139