TargetLowering: fix an infinite DAG combine in SimplifySETCC

[jroelofs]

Summary

TargetLowering::SimplifySetCC wants to swap the operands of a SETCC to canonicalize the constant to the RHS. The bug here was that it did so whether or not the RHS was already a constant, leading to an infinite loop.

rdar://111847838

[jroelofs]

llvm/test/CodeGen/AArch64/arm64-setcc-swap-infloop.ll

26

The bug is really sensitive to the UB here. Removing any of the UB on these first 3 lines makes the bug go away.

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4230

Doesn't this change prevent folding constant comparisons? But maybe that's handled by the FoldSetCC above? Was that added later than this comment was written?

[jroelofs]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4230

Looks like this commit is the original source of that comment:

https://github.com/llvm/llvm-project/commit/65919b5058e155df43c248a18b5ecba52bd4d20f

I don't follow how this ever facilitated constant folding. But yes, looks like FoldSetCC was added later, and it takes care of those duties. I'll update the comment.

[craig.topper]

LGTM

[jroelofs]

Add a carveout for the cases the RISCV tests care about.

[jroelofs]

More precise carveout.

[nikic]

What is the affected RISCV test case?

[jroelofs]

llvm/test/CodeGen/RISCV/rvv/setcc-integer.ll

https://lab.llvm.org/buildbot#builders/16/builds/51283

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4237

I'm not sure about the logic of this carve out. splat_vector is primarily used with scalable vectors. So checking it is not a scalable vector, but N0 is a splat_vector seems odd.

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4237

I think the check you want is just N0ConstOrSplat && !N1ConstOrSplat. The old code only checked !N1ConstOrSplat for scalable vectors. Now you want to check it regardless of scalable vectors.

[jroelofs]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4237

I think the check you want is just N0ConstOrSplat && !N1ConstOrSplat.

That's what the landed-but-reverted commit [1] did, but it broke some RISCV tests that IIUC are expecting that (setcc (splat (const)), (buildvector (const))) get normalized to put the constant splat on the RHS (so that subsequent combine(s) can then constant-fold that, I think?).

1: https://reviews.llvm.org/D155095?id=539663

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4237

I mean remove any mention of !OpVT.isScalableVector.

[jroelofs]

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

4237

Oh, makes sense. I think that works.

[jroelofs]

@craig.topper's feedback

[craig.topper]

LGTM

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

   if (SDValue Fold = DAG.FoldSetCC(VT, N0, N1, Cond, dl))
     return Fold;
 
   bool N0ConstOrSplat =
       isConstOrConstSplat(N0, /*AllowUndefs*/ false, /*AllowTruncate*/ true);
   bool N1ConstOrSplat =
       isConstOrConstSplat(N1, /*AllowUndefs*/ false, /*AllowTruncate*/ true);
 
-  // Ensure that the constant occurs on the RHS and fold constant comparisons.
+  // Canonicalize toward having the constant on the RHS.
   // TODO: Handle non-splat vector constants. All undef causes trouble.
   // FIXME: We can't yet fold constant scalable vector splats, so avoid an
   // infinite loop here when we encounter one.
   ISD::CondCode SwappedCC = ISD::getSetCCSwappedOperands(Cond);
-  if (N0ConstOrSplat && (!OpVT.isScalableVector() || !N1ConstOrSplat) &&
+  if (N0ConstOrSplat && !N1ConstOrSplat &&
       (DCI.isBeforeLegalizeOps() ||
        isCondCodeLegal(SwappedCC, N0.getSimpleValueType())))
     return DAG.getSetCC(dl, VT, N1, N0, SwappedCC);
 
   // If we have a subtract with the same 2 non-constant operands as this setcc
   // -- but in reverse order -- then try to commute the operands of this setcc
   // to match. A matching pair of setcc (cmp) and sub may be combined into 1
   // instruction on some targets.
   if (!N0ConstOrSplat && !N1ConstOrSplat &&
       (DCI.isBeforeLegalizeOps() ||

llvm/test/CodeGen/AArch64/arm64-setcc-swap-infloop.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=arm64-apple-ios --global-isel=0 | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+target triple = "arm64-apple-ios"
+
+declare void @llvm.memset.p0.i64(ptr nocapture writeonly, i8, i64, i1 immarg)
+
+; TargetLowering::SimplifySetCC wants to swap the operands of a SETCC to
+; canonicalize the constant to the RHS. The bug here was that it did so whether
+; or not the RHS was already a constant, leading to an infinite loop.
+define <16 x i1> @setcc_swap_infloop(ptr %arg) {
+; CHECK-LABEL: setcc_swap_infloop:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    mov x8, xzr
+; CHECK-NEXT:    mov w9, #16 ; =0x10
+; CHECK-NEXT:    movi.16b v1, #1
+; CHECK-NEXT:    ldr q0, [x8]
+; CHECK-NEXT:    cmeq.16b v2, v1, #0
+; CHECK-NEXT:    str q1, [x8]
+; CHECK-NEXT:    cmeq.16b v0, v0, #0
+; CHECK-NEXT:    str q1, [x9]
+; CHECK-NEXT:    orr.16b v0, v0, v2
+; CHECK-NEXT:    ret
+  call void @llvm.memset.p0.i64(ptr nonnull null, i8 1, i64 32, i1 false)
+  %v = getelementptr inbounds i8, ptr null, i64 16
+  %v14 = load <16 x i8>, ptr undef, align 32
+  %v15 = icmp eq <16 x i8> %v14, zeroinitializer
+  %v16 = load <16 x i8>, ptr %v, align 16
+  %v17 = icmp eq <16 x i8> %v16, zeroinitializer
+  %v20 = or <16 x i1> %v15, %v17
+  ret <16 x i1> %v20
+}