[PATCH] D82499: [DAGCombiner] tighten constraints for fma fold

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D82499#2329454 <https://reviews.llvm.org/D82499#2329454>, @qiucf wrote:

> Hi, I see `reassoc` and `contract` are equivalent in many cases (at least in DAG combiner):
>
>   define double @foo(double %a, double %b, double %c) {
>   entry:
>     %0 = fmul reassoc double %a, %b
>     %1 = fadd reassoc double %0, %c
>     ret double %1
>   }
>   ; => no contract bit, it's still fused into fma
>
>
>
>   define double @foo2(double %a, double %b) {
>   entry:
>     %0 = fmul double %a, 1.1
>     %1 = call contract double @llvm.fma.f64(double %0, double 2.1, double %b)
>     ret double %1
>   }
>   ; => no reassoc set, (fma (fmul x, c1), c2, y) became (fma x, c1*c2, y)
>   
>   declare double @llvm.fma.f64(double, double, double)
>
> Is this expected, or we need to differentiate them in more places?

In the first example, I think of `reassoc` as including anything that `contract` could enable, so `reassoc` is a superset. If that's not the right model, we should adjust the documentation to allow the other interpretation. It does look like gcc makes some distinction between "unsafe" and FP-contraction, but it does not behave the way I was expecting:
https://godbolt.org/z/EfhPYT

The second example looks very similar to the bug we fixed with this patch: we are not allowed to reassociate the order of multiplications with `contract` alone, so that's a bug.
Let me know if I should post a patch for that or if you are already working on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82499