[PATCH] D45453: [InstCombine] Enable Add/Sub simplifications with only 'reassoc' FMF

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

A couple of short comments/questions, and one longer one:

In https://reviews.llvm.org/D45453#1063225, @spatel wrote:

> ... the remaining big blob is FAddCombine() - which is poorly named because as shown in this patch, it's also called from visitFSub()...


Should we rename it to FAddSubCombine() in this work (assuming this patch moves forward, given the other questions here)?

> ... I'd still like to have dedicated tests in instcombine for whatever is happening in FAddCombine() though, so add to the tests from https://reviews.llvm.org/rL170471 to demonstrate at least some of the diffs without needing -reassociate?

To be sure I'm understanding, you mean demonstrate some diffs in those tests using the flag 'reassoc' rather than 'fast' (without needing -reassociate), right?  I have a concern that meaningful transformations based on the 'reassoc' flag alone may require both -instcombine and -reassociate.

My longer question relates to:

> ... but if others are confident that it's just reassociation (without need for nsz/nnan/ninf), then I suppose this is ok to loosen up.

In a sense, I do feel we should check 'nsz' in addition to 'reassoc' (and maybe others).  And similarly, the tests in the other files that I modified in this patch possibly should only do the transformation if 'reassoc' and 'nsz' (and others?) are set.  But when starting with C/C++ (when `-ffast-math` isn't specified), we only will set 'reassoc' if some other switches in addition to `-fassociative-math` are passed.  Specifically:
`-fassociative-math -fno-signed-zeros -fno-trapping-math`

(details at https://reviews.llvm.org/D39812).

So in a sense, an instruction that has only 'reassoc' set on it isn't complete, in that it cannot be generated by Clang.  I feel like this is an IEEE thing, rather than a C/C++ thing (so other language front-ends ought to do something similar).  But from this perspective, is checking only 'reassoc' OK?  Or is should we change tests that have only 'reassoc' on an instruction to also have 'nsz'?  Which then leads me to the problem/question that `-fno-trapping-math` doesn't have a bit in FMF, so it creates a new dimension to the problem/question.

Anyway, I took the interpretation that when the 'reassoc' bit is set, it in a sense means the equivalent of at least `-fassociative-math -fno-signed-zeros -fno-trapping-math` from a C/C++ command-line perspective.  I'm not sure how we should interpret this.  As I write this comment, I feel like I've opened a can of worms.


https://reviews.llvm.org/D45453