Page MenuHomePhabricator

[InstSimplify] allow exp/log simplifications with only 'reassoc' FMF
ClosedPublic

Authored by spatel on Feb 10 2018, 8:56 AM.

Details

Summary

As discussed on llvm-dev recently, we should refine our FP transforms based on the minimum subset of fast-math-flags needed to enable those transforms.

These intrinsic folds were just added with D41381, but we let them slide with isFast(). I think 'reassoc' is enough given that we're just eliminating ops based on their math inverses.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Feb 10 2018, 8:56 AM

Code change LGTM. A minor tweak to each test-change (to replace 'fast' with 'reassoc') would test it more precisely.

test/Transforms/InstSimplify/exp-intrinsic.ll
22 ↗(On Diff #133753)

'reassoc' instead of 'fast' should also be all that's needed for the llvm.log.f64 call to enable this transformation.

test/Transforms/InstSimplify/exp2-intrinsic.ll
22 ↗(On Diff #133753)

Again, 'reassoc' is all that's needed.

test/Transforms/InstSimplify/log-intrinsic.ll
22 ↗(On Diff #133753)

'reassoc' is all that's needed here, too.

test/Transforms/InstSimplify/log2-intrinsic.ll
22 ↗(On Diff #133753)

One last one where 'reassoc' is all that's needed.

spatel added inline comments.Feb 12 2018, 11:47 AM
test/Transforms/InstSimplify/exp-intrinsic.ll
22 ↗(On Diff #133753)

Hmm...looking a bit closer, this test doesn't add much value. We show that the FMF on the first instruction is irrelevant in the next test (and the same is true in each file). Should I just delete this and its siblings?

wristow added inline comments.Feb 12 2018, 2:24 PM
test/Transforms/InstSimplify/exp-intrinsic.ll
22 ↗(On Diff #133753)

Now that you mention it, yes, that makes more sense.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 12 2018, 3:53 PM
This revision was automatically updated to reflect the committed changes.