[AArch64] Fix performPostLD1Combine to check for constant lane index.

[gberry]

Summary

performPostLD1Combine in AArch64ISelLowering looks for vector
insert_vector_elt of a loaded value which it can optimize into a single
LD1LANE instruction. The code checking for the pattern was not checking
if the lane index was a constant which could cause two problems:

• an assert when lowering the LD1LANE ISD node since it assumes an constant operand

• an assert in isel if the lane index value depends on the post-incremented base register

Both of these issues are avoided by simply checking that the lane index
is a constant.

Fixes bug 35822.

[samparker]

lib/Target/AArch64/AArch64ISelLowering.cpp

9938 ↗

(On Diff #145730)

Should the value of Lane also be checked for legality?

[javed.absar]

lib/Target/AArch64/AArch64ISelLowering.cpp

9938 ↗

(On Diff #145730)

Maybe simpler to write it as :

// Check lane is a ...
if (IsLaneOp && !isa<ConstantSDNode>(N->getOperand(2))

return ..

[gberry]

Add lane index range check

[gberry]

lib/Target/AArch64/AArch64ISelLowering.cpp

9938 ↗

(On Diff #145730)

I added a range check of the lane index, even though I don't think it is possible to get an INSERT_VECTOR_ELT with an out-of-range constant index at this point in isel. Better safe than sorry though.

@javed.absar I don't think your suggestion makes sense after the addition of the range check. I also like that the later use of 'Lane' is a little clearer, but I don't feel strongly about it.

[samparker]

LGTM, thanks!

llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp

static SDValue performPostLD1Combine(SDNode *N,
   EVT VT = N->getValueType(0);
 
   unsigned LoadIdx = IsLaneOp ? 1 : 0;
   SDNode *LD = N->getOperand(LoadIdx).getNode();
   // If it is not LOAD, can not do such combine.
   if (LD->getOpcode() != ISD::LOAD)
     return SDValue();
 
+  // The vector lane must be a constant in the LD1LANE opcode.
+  SDValue Lane;
+  if (IsLaneOp) {
+    Lane = N->getOperand(2);
+    auto *LaneC = dyn_cast<ConstantSDNode>(Lane);
+    if (!LaneC || LaneC->getZExtValue() >= VT.getVectorNumElements())
+      return SDValue();
+  }
+
   LoadSDNode *LoadSDN = cast<LoadSDNode>(LD);
   EVT MemVT = LoadSDN->getMemoryVT();
   // Check if memory operand is the same type as the vector element.
   if (MemVT != VT.getVectorElementType())
     return SDValue();
 
   // Check if there are other uses. If so, do not combine as it will introduce
   // an extra load.
for (SDNode::use_iterator UI = Addr.getNode()->use_begin(), UE =
     // LD1*post we'll eventually generate anyway.
     if (LoadSDN->isPredecessorOf(Vector.getNode()))
       continue;
 
     SmallVector<SDValue, 8> Ops;
     Ops.push_back(LD->getOperand(0));  // Chain
     if (IsLaneOp) {
       Ops.push_back(Vector);           // The vector to be inserted
-      Ops.push_back(N->getOperand(2)); // The lane to be inserted in the vector
+      Ops.push_back(Lane);             // The lane to be inserted in the vector
     }
     Ops.push_back(Addr);
     Ops.push_back(Inc);
 
     EVT Tys[3] = { VT, MVT::i64, MVT::Other };
     SDVTList SDTys = DAG.getVTList(Tys);
     unsigned NewOp = IsLaneOp ? AArch64ISD::LD1LANEpost : AArch64ISD::LD1DUPpost;
     SDValue UpdN = DAG.getMemIntrinsicNode(NewOp, SDLoc(N), SDTys, Ops,

llvm/trunk/test/CodeGen/AArch64/arm64-indexed-vector-ldst-2.ll

 if.then172:                                       ; preds = %cond.end90
   %7 = tail call i64 @llvm.objectsize.i64.p0i8(i8* undef, i1 false)
   br label %return
 
 return:                                           ; preds = %if.then172, %cond.end90, %entry
   ret void
 }
 
+; Avoid an assert/bad codegen in LD1LANEPOST lowering by not forming
+; LD1LANEPOST ISD nodes with a non-constant lane index.
+define <4 x i32> @f2(i32 *%p, <4 x i1> %m, <4 x i32> %v1, <4 x i32> %v2, i32 %idx) {
+  %L0 = load i32, i32* %p
+  %p1 = getelementptr i32, i32* %p, i64 1
+  %L1 = load i32, i32* %p1
+  %v = select <4 x i1> %m, <4 x i32> %v1, <4 x i32> %v2
+  %vret = insertelement <4 x i32> %v, i32 %L0, i32 %idx
+  store i32 %L1, i32 *%p
+  ret <4 x i32> %vret
+}
+
+; Check that a cycle is avoided during isel between the LD1LANEPOST instruction and the load of %L1.
+define <4 x i32> @f3(i32 *%p, <4 x i1> %m, <4 x i32> %v1, <4 x i32> %v2) {
+  %L0 = load i32, i32* %p
+  %p1 = getelementptr i32, i32* %p, i64 1
+  %L1 = load i32, i32* %p1
+  %v = select <4 x i1> %m, <4 x i32> %v1, <4 x i32> %v2
+  %vret = insertelement <4 x i32> %v, i32 %L0, i32 %L1
+  ret <4 x i32> %vret
+}
+
 ; Function Attrs: nounwind readnone
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1) #1
 
 attributes #0 = { nounwind ssp "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
 attributes #1 = { nounwind readnone }
 
 !1 = !{!2, !2, i64 0}
 !2 = !{!"double", !3, i64 0}
 !3 = !{!"omnipotent char", !4, i64 0}
 !4 = !{!"Simple C/C++ TBAA"}