[PATCH] D73939: [AMDGPU] Fix infinite loop with fma combines

[Austin Kerbow via Phabricator via llvm-commits]

kerbowa added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:742
+  switch (Op.getOpcode()) {
+    case ISD::FMA: {
+      // Negating a fma is not free if it has users without source mods.
----------------
arsenm wrote:
> kerbowa wrote:
> > arsenm wrote:
> > > kerbowa wrote:
> > > > arsenm wrote:
> > > > > This is incomplete and FMAD will have the same problem at minimum
> > > > The hang at least doesn't effect FMAD. The combine that triggers this wont happen.
> > > Where is the combine? It's also suspicious if an fneg combine triggered for FMA but not FMAD. 
> > It's a loop so there are combines in two directions.
> > 
> > The FNeg combine -fma(a, b, c) => fma(a, -b, -c), triggers for FMA/FMAD and is correct.
> > 
> > AFAICS the unprofitable combine, fma(a, -b, -c) => -fma(a, b, c), only triggers for FMA.
> > https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L12652
> This should be just as valid for fmad, but we probably don't want it. This should handle fmad as well.
> 
> I am somewhat concerned about splitting much of the same logic between isNegatibleForFree and the target specific fneg combines. Is there now more overlap in what happens in the AMDGPU performFNegCombine and DAGCombiner than there used to be?
It might just be this one combine that was added. I agree with you though that there is danger that the logic becomes unnecessarily split, but the only other short term option is reverting https://reviews.llvm.org/D72312.

I know not many targets are overriding isNegatibleForFree. In this case, the generic rule is that if fnegs are not free, then two is worse then one. I.e. -fma(a, b, c) is better than fma(a, -b, -c). It does seem like we need target specific logic here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73939