[PATCH] D113603: [x86] fold vector (X > -1) & Y to shift+andn

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 4 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:45812
+  // requires "and" rather than "andn":
+  // and (pcmpgt X, -1), Y --> pandn (sra X, BW-1), Y
+  if (supportedVectorShiftWithImm(VT.getSimpleVT(), Subtarget, ISD::SRA)) {
----------------
craig.topper wrote:
> pengfei wrote:
> > What's the BW mean? Byte and word? But the tests show for i16 and i32.
> I assume it’s BitWidth
Yes, that was an abbreviation for BitWidth that I've used in other places, but that's less clear here with x86's lingo in play. I'll spell that out to be less confusing.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:45817
+        isAllOnesOrAllOnesSplat(Op1.getOperand(1)))
+      std::swap(Op0, Op1);
+
----------------
RKSimon wrote:
> I know this shouldn't technically fall through, but its still a little scary to be commuting Op0/Op1 like this - maybe just duplicate fold?
I can make those block-local-variables with better names to avoid risk.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:45823
+      SDValue ShAmtC = DAG.getConstant(VT.getScalarSizeInBits() - 1, DL, VT);
+      SDValue Sra = DAG.getNode(ISD::SRA, DL, VT, Op0.getOperand(0), ShAmtC);
+      return DAG.getNode(X86ISD::ANDNP, DL, VT, Sra, Op1);
----------------
RKSimon wrote:
> Use getTargetVShiftByConstNode ?
Sure, looks like we can go directly to an x86 node at this point without changing any results.


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

https://reviews.llvm.org/D113603