[PATCH] D40696: Enable aggressive FMA on T99 and provide AArch64 option for other micro-arch's

[Stefan Teleman via Phabricator via llvm-commits]

steleman added inline comments.


================
Comment at: test/CodeGen/AArch64/fma-aggressive.ll:323
+
+attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="true" "no-jump-tables"="false" "no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="thunderx2t99" "target-features"="+lse,+neon,+v8.1a" "unsafe-fp-math"="true" "use-soft-float"="false" }
+attributes #1 = { argmemonly nounwind }
----------------
fhahn wrote:
> steleman wrote:
> > fhahn wrote:
> > > Can we drop the meta data from here to the end or is it required for the test?
> > I'll check and if it works without the metadata, I'll remove it.
> Great thanks. On second look, I think there is no need for the test case to be that complicated, for example, do we need all those global variables, function calls, ifs, loops? AFAIK DagCombine only looks at nodes and uses and does not have a cost model tied to loop iterations. 
> 
> Unless I am missing something, it should be enough to have a couple of basic blocks with fadd/fmul instructions to test this, e.g. fadd/fmul with multiple users, or the more complex patterns guarded by `if (Aggressive)` in `visitFADDForFMACombine`.
> 
> A much simpler test case will make it easier to debug if it fails in the future and also easier to understand when reviewing if it does the right thing.

It's not as simple as you think.

DAGCombiner.cpp:9636


```
  // fold (fmul (fadd x, +1.0), y) -> (fma x, y, y)
  // fold (fmul (fadd x, -1.0), y) -> (fma x, y, (fneg y))
  auto FuseFADD = [&](SDValue X, SDValue Y) {
    if (X.getOpcode() == ISD::FADD && (Aggressive || X->hasOneUse())) {
      auto XC1 = isConstOrConstSplatFP(X.getOperand(1));
      if (XC1 && XC1->isExactlyValue(+1.0))
        return DAG.getNode(PreferredFusedOpcode, SL, VT, X.getOperand(0), Y, Y);
      if (XC1 && XC1->isExactlyValue(-1.0))
        return DAG.getNode(PreferredFusedOpcode, SL, VT, X.getOperand(0), Y,
                           DAG.getNode(ISD::FNEG, SL, VT, Y));
    }
    return SDValue();
  };

```

Note:


```
if (X.getOpcode() == ISD::FADD && (Aggressive || X->hasOneUse()))
```

What we want to do here is make sure that X has more than one use. I.e. X->hasOneUse() evaluates to false. That's the only way of testing that FMA happens when Aggressive is true.

In order to make sure that X ends up having more than one use, the code needs to be complicated.

This code isn't lifted from some existing program. I wrote it specifically for this purpose. If there was no need for these loops, ifs, global variables, etc, I wouldn't have written them.




Repository:
  rL LLVM

https://reviews.llvm.org/D40696