[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:
> > 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.
> hi, dmgreen
>    In fact, I can't reproduce the failure base on my local building with delete the condition !RHS.hasOneUse(), so it may be not this change cause the regression, as you can see the other person's commit don't also pass building recently.
> 
>    Just to be sure, do you think it is ok to roll back the change of delete the condition !RHS.hasOneUse(), and submit it firstly ? I'll resubmit the change of delete the condition !RHS.hasOneUse() after the upstream is stable with double checking.
> 
> 
> The building log is finally came out record in https://reviews.llvm.org/harbormaster/build/215411/, and detail info can be found in [[ https://buildkite.com/llvm-project/premerge-checks/builds/76065#e9958d49-b20c-4465-ba63-7c9372fb5b4f | x86 debian fail ]]
> Failed Tests (9):
>   libarcher :: races/critical-unrelated.c
>   libarcher :: races/lock-nested-unrelated.c
>   libarcher :: races/lock-unrelated.c
>   libarcher :: races/parallel-simple.c
>   libarcher :: races/task-dependency.c
>   libarcher :: races/task-taskgroup-unrelated.c
>   libarcher :: races/task-taskwait-nested.c
>   libarcher :: races/task-two.c
>   libarcher :: task/task_late_fulfill.c
They sound like they should be unrelated, coming from a x86 machine. I would recommend the version without the !RHS.hasOneUse() check, unless we have some evidence that it is really this check that is breaking something. But I don't think it should do.


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

https://reviews.llvm.org/D116915