[PATCH] D104247: [DAGCombine] reassoc flag shouldn't enable contract

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

In D104247#2828581 <https://reviews.llvm.org/D104247#2828581>, @pengfei wrote:

> In D104247#2827116 <https://reviews.llvm.org/D104247#2827116>, @jsji wrote:
>
>> In D104247#2827089 <https://reviews.llvm.org/D104247#2827089>, @spatel wrote:
>>
>>> In code that I've looked at (mostly C compiled with -ffast-math), we always have `contract` when we have `reassoc`, so I don't see much practical difference.
>>> Can you explain more how we could benefit from this change - in the example in the description, we would have 4 instructions rather than 3 if we use FMA - is that better?
>>
>> Yes, you are right, the performance of having FMA should be better. However, we have quite some scenarios that users care about precision more than performance, they want to precise control of when FMA can be generated. So The major motivation of this is to ensure that we respect the IR semantics. For users that care about performance, we still can get them through default global option or emitting respect flag in IR.
>
> AFAIU, the use of FMA doesn't decline the precision (but may improve a bit, which is also unexpected in some circumstance).

Yes, you are right. FMA on PowerPC improves precision, which is sometime unexpected to math library writers.

But `reassoc` flag does affect precision much.

> We had some discussion on D99675 <https://reviews.llvm.org/D99675>, `contract` is acceptable even we want to disable `reassoc` for the sake of precision. 
> That said, `contract` has much less influence on precision than `reassoc`. So I don't see any necessity to use `reassoc` without `contract`.

I agree in general.  
However, there are some specific cases that IR producer might generate `reassoc` flag for general reassociation, not intending for `contract`, 
and current implementation is causing confusions and failures.

As  `reassoc` and `contract` are defined as two independent flags,  
I think we should distinguish them in implementation, regardless of whether it is really meaningful to  use `reassoc` without `contract`?

Or another choice is we can update the definition of these two flags, saying that `reassoc` always imply `contract`,
but this doesn't looks a clear definition to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104247