[PATCH] D116915: [DAGCombiner][AArch64] Enhance to support for scalar CSINC

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14630
+
+  if (!CSel.hasOneUse() || !RHS.hasOneUse())
+    return SDValue();
----------------
Allen wrote:
> dmgreen wrote:
> > I don't think it matters if the RHS has more than 1 use, it should still be OK to transform as we only use the value in another expression.
> I had delete the condition !RHS.hasOneUse(), but recently  the upstream may has some regression, it doen't finish the precommit build checking.  so may  I need wait a moment  to figure out that issue ?
Hmm. That sounds suspicious. What was the problem? That would usually imply an infinite loop in DAG combine, but I'm not sure how `csinc(add(x, y), x, cc)` could get back to `add(x, csel(y, 1, cc))`. Or why the uses of y matter in that case.

I was about to OK the revision, but it would be good to make sure that nothing problematic is happening.


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

https://reviews.llvm.org/D116915