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

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs marked an inline comment as not done.
jroelofs 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) &&
----------------
craig.topper wrote:
> 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.
Oh, makes sense. I think that works.


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

https://reviews.llvm.org/D155095