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

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - see inline for a couple of potential follow-ups.



================
Comment at: test/Transforms/InstCombine/fast-math.ll:407-414
+; Check again using the minimal subset of FMF (reassoc is sufficient).
+define float @fold14_reassoc(float %x, float %y) {
+; CHECK-LABEL: @fold14_reassoc(
+; CHECK-NEXT:    [[ADD:%.*]] = fsub reassoc float [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    ret float [[ADD]]
+;
+  %neg = fsub reassoc float -0.0, %x
----------------
wristow wrote:
> spatel wrote:
> > -x + y --> y - x
> > 
> > This doesn't require any FMF.
> Good point!  In fact, it //does //fold without any FMF, even without my patch.  I've updated the test to demonstrate the folding without 'fast' (or anything else).
Nit: it's a bit crazy that there are no existing regression tests for these always-safe fneg folds; must be from the early cowboy compiler days. :)
There really should be an 'fadd.ll' test file for non-FMF folds.


================
Comment at: test/Transforms/InstCombine/fast-math.ll:392
+;
+  %mul = fmul reassoc nsz float %x, 7.000000e+00
+  %sub = fsub reassoc nsz float %mul, %x
----------------
Maybe independent of this patch, but the fmul shouldn't need any FMF for this fold, so it's worth checking if that actually happens. Another consideration is whether the fmul hasOneUse(). We definitely don't have enough multi-use tests to show that folds are not unintentionally creating extra instructions.


https://reviews.llvm.org/D45453