[PATCH] D143785: [X86] Add Extend shuffle pattern to vNf32 shuffles.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:14326
+    if (InputV.getValueType() != VT)
+      InputV = DAG.getBitcast(VT, InputV);
     InputV = ShuffleOffset(InputV);
----------------
Don't bother with the if() test - getBitcast will do that internally anyway:

InputV = DAG.getBitcast(VT, InputV);


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:14334-14335
   assert(VT.is128BitVector() && "Only 128-bit vectors can be extended.");
+  if (InputV.getValueType() != VT)
+    InputV = DAG.getBitcast(VT, InputV);
 
----------------
Drop the if() and always call getBitcast


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:16979
+  if (!Lo || !Hi)
+    return SDValue();
   return DAG.getNode(ISD::CONCAT_VECTORS, DL, VT, Lo, Hi);
----------------
We've had problems before where we've created nodes before bailing out later on - causing various problems including hasOneUse assumption failures later on.

Is there any nice way that we can do the SimpleOnly checks BEFORE calling HalfBlend? Or maybe split into an analysis lambda and a getVectorShuffle lambda.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18387
+        M = (M % LaneSize) + ((i / LaneSize) * LaneSize) + Size;
+    }
+
----------------
Pull this out into a helper function?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:18398
+    ZExt = DAG.getBitcast(MVT::v8f32, ZExt);
+    return ZExt;
+  }
----------------
return DAG.getBitcast(MVT::v8f32, ZExt);


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:19135
+    ZExt = DAG.getBitcast(MVT::v16f32, ZExt);
+    return ZExt;
+  }
----------------
return DAG.getBitcast(MVT::v16f32, ZExt);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143785