[PATCH] D78895: [InstCombine][SVE] Fix visitInsertElementInst for scalable type.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for working on this @huihuiz! I've left some comments on the patch.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1059
   Value *ExtVecOp;
-  if (match(IdxOp, m_ConstantInt(InsertedIdx)) &&
+  if (!isa<ScalableVectorType>(IE.getType()) &&
+      match(IdxOp, m_ConstantInt(InsertedIdx)) &&
----------------
nit: `!isa<ScalableVectorType>` -> `isa<FixedVectorType>`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1064
+      ExtractedIdx <
+          cast<FixedVectorType>(ExtVecOp->getType())->getNumElements()) {
     // TODO: Looking at the user(s) to determine if this insert is a
----------------
This is missing a `&& isa<FixedVectorType>(ExtVecOp->getType())`, because the vector that the value is extracted from could be scalable.
(which should be guarded by an extra test case)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1108
+    APInt AllOnesEltMask(APInt::getAllOnesValue(VWidth));
+    if (Value *V = SimplifyDemandedVectorElts(&IE, AllOnesEltMask, UndefElts)) {
+      if (V != &IE)
----------------
This probably also needs a change in `SimplifyDemandedVectorElts` to bail out early on scalable vectors.


================
Comment at: llvm/test/Transforms/InstCombine/vscale_insertelement.ll:4
+
+define <vscale x 4 x float> @insertelement_bitcast(<vscale x 4 x i32> %vec, i32 %x) {
+; CHECK-LABEL: @insertelement_bitcast(
----------------
nit: Can you add a comment what this is testing <=> what it is trying to prevent?


================
Comment at: llvm/test/Transforms/InstCombine/vscale_insertelement.ll:17
+; This test checks that we are not crashing at:
+; "Assertion `isValidOperands(V1, V2, Mask) && "Invalid shuffle vector instruction operands!"' failed."
+define <vscale x 4 x i32> @insertelement_extractelement(<vscale x 4 x i32> %a, <vscale x 4 x i32> %b) {
----------------
nit: This comment doesn't explain what causes the invalid isValidOperands to occur. Can you explain what code-path/optimization you're trying to prevent in this test?


================
Comment at: llvm/test/Transforms/InstCombine/vscale_insertelement.ll:30
+; This test checks that we are not crashing at:
+; "Assertion `isValidOperands(V1, V2, Mask) && "Invalid shuffle vector instruction operands!"' failed."
+define <vscale x 4 x i32> @insertelement_insertelement(<vscale x 4 x i32> %vec) {
----------------
Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78895