[PATCH] D48789: [X86] Replace (32/64 - n) shift amounts with (neg n) since the shift amount is masked in hardware

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Other than nits i have no further comments, thanks.



================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2698-2699
+
+  // Special case to avoid messing up a BZHI pattern.
+  // Look for (srl (shl X, (size - y)), (size - y)
+  if (Subtarget->hasBMI2() && (VT == MVT::i32 || VT == MVT::i64) &&
----------------
craig.topper wrote:
> lebedev.ri wrote:
> > Why? I would think we can simply add one more pattern to the `BZHI`?
> > I think this is bad because it not only not "messing up a BZHI pattern",
> > but anything else too, since it does not check that the user is `and %val, %mask`.
> This code runs when the root node considered for isel is a srl/sra/shl. And it runs before any tablegen patterns on that node.  Any patterns that used a shift, but where the shift was not the root node have already been matched.
> 
> So I'm specifically blocking out this one bzhi pattern that has a shift as a root node.
> 
> ```
>     // x << (bitwidth - y) >> (bitwidth - y)
>     defm : _bmi_bzhi_pattern<(srl (shl RC:$src, (sub bitwidth, GR8:$lz)),
>                                   (sub bitwidth, GR8:$lz)),
>                              (srl (shl (x86memop addr:$src),
>                                         (sub bitwidth, GR8:$lz)),
>                                   (sub bitwidth, GR8:$lz)),
>                              RC, VT, DstInst, DstMemInst>;
> ```
I see, thank you for the explanation.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2715
+    SDValue Add1 = ShiftAmt->getOperand(1);
+    // If we are shifting by X+/-N where N == 0 mod Size, then just shift by X
+    // to avoid the ADD/SUB.
----------------
Are you sure this shouldn't be `N mod Size == 0`?
At least i think that is what the code does.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2720
+      NewShiftAmt = Add0;
+    // If we are shifting by N-X where N == 0 mod Size, then just shift by -X to
+    // generate a NEG instead of a SUB of a constant.
----------------
Same


Repository:
  rL LLVM

https://reviews.llvm.org/D48789