[PATCH] D155095: TargetLowering: fix an infinite DAG combine in SimplifySETCC

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4237
   ISD::CondCode SwappedCC = ISD::getSetCCSwappedOperands(Cond);
-  if (N0ConstOrSplat && (!OpVT.isScalableVector() || !N1ConstOrSplat) &&
+  if (N0ConstOrSplat && ((!OpVT.isScalableVector() && N0Splat && !N1Splat) ||
+                         !N1ConstOrSplat) &&
----------------
jroelofs wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > 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.
> > 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.
> > 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
I mean remove any mention of !OpVT.isScalableVector.


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

https://reviews.llvm.org/D155095