Page MenuHomePhabricator

[IR] Add AllowContract to FastMathFlags
ClosedPublic

Authored by anemet on Mar 20 2017, 8:08 PM.

Details

Summary

-ffp-contract=fast does not currently work with LTO because it's passed as a
TargetOption to the backend rather than in the IR. This adds it to
FastMathFlags.

This is toward fixing PR25721

Diff Detail

Repository
rL LLVM

Event Timeline

anemet created this revision.Mar 20 2017, 8:08 PM
arsenm added a subscriber: arsenm.Mar 20 2017, 8:25 PM

Needs assembler and bitcode compatibility tests

docs/LangRef.rst
2197–2200 ↗(On Diff #92420)

Should we also have an unsafe algebra that preserves signed zeros/nans etc?

mehdi_amini added inline comments.Mar 20 2017, 8:30 PM
docs/LangRef.rst
2197–2200 ↗(On Diff #92420)

@arsenm : isn't it the default when there is no nsz or nnan ?

2199 ↗(On Diff #92420)

Is it the intent to allow this as well : x + x + x -> 3 * x ?

arsenm added inline comments.Mar 20 2017, 8:43 PM
docs/LangRef.rst
2197–2200 ↗(On Diff #92420)

No, that is normal safe math. I mean unsafe algebraic transformations similar to contraction which will preserve nan/signed zeros/inf behavior but are still not safe.

anemet added inline comments.Mar 20 2017, 8:51 PM
docs/LangRef.rst
2199 ↗(On Diff #92420)

I don't think so. We do the general case (any number of terms) under unsafe-math, in FAddCombine::simplify in InstCombine.

anemet added inline comments.Mar 20 2017, 9:07 PM
docs/LangRef.rst
2197–2200 ↗(On Diff #92420)

Maybe but we may want that separate from contraction where the result is more exact.

mehdi_amini added inline comments.Mar 20 2017, 9:09 PM
docs/LangRef.rst
2197–2200 ↗(On Diff #92420)

Do you have an example of such transformations?

mehdi_amini added inline comments.Mar 20 2017, 9:10 PM
docs/LangRef.rst
2199 ↗(On Diff #92420)

I know we do this under unsafe-fp-math, I was wondering specifically about how fat the "contract" mode allows to go.

anemet added inline comments.Mar 20 2017, 9:32 PM
docs/LangRef.rst
2199 ↗(On Diff #92420)

Ah ok, sorry, I misunderstood. The C standard frames FP_CONTRACT as a permission to omit rounding errors while contracting, so potentially this may be included too.

spatel added a subscriber: spatel.Mar 21 2017, 8:14 AM
spatel added inline comments.
docs/LangRef.rst
2197–2200 ↗(On Diff #92420)

If I'm understanding Matt's question, a mode that allows reassociation but is independent of the other relaxation bits could be handled by not having "fast" imply all of the other bits. This came up here:
http://lists.llvm.org/pipermail/llvm-dev/2016-November/107104.html

I think we need that mode, but it's separate from this set of patches.

scanon added a subscriber: scanon.Mar 21 2017, 9:42 AM
scanon added inline comments.
docs/LangRef.rst
2199 ↗(On Diff #92420)

Fun fact, x + x + x -> 3*x is always legal (both expressions only have a single rounding). x + x + x + x -> 4*x requires the assumption of default rounding, however.

anemet added inline comments.Mar 21 2017, 10:43 AM
docs/LangRef.rst
2197–2200 ↗(On Diff #92420)

@spatel, thanks for the context. I agree that is a different topic from this set of patches. Here the goal is to represent -ffp-contract=fast in the IR without changing semantics or its relation to -ffast-math ('fast' implies contraction).

anemet updated this revision to Diff 92521.Mar 21 2017, 11:48 AM

Address Matt's comments

spatel added inline comments.Mar 27 2017, 12:37 PM
include/llvm/IR/Operator.h
177–178 ↗(On Diff #92521)

This comment isn't right - FMF is using "SubclassOptionalData" not "SubclassData".

Not sure if it's worth mentioning as a code comment, but we only have room for one more FMF bit if we keep using SubclassOptionalData; it only has 7 bits total.

209–211 ↗(On Diff #92521)

If we want to add a bool param, should we do that for all of the flags as a follow-up change? Seems odd to make setAllowContract() different than everything else if there's no change to use a 'false' setter in this patch.

319–320 ↗(On Diff #92521)

I know it's copying the format of the other comments, but when do we use these single-letter abbreviations of the FMF? If never, just remove these comments?

anemet marked 2 inline comments as done.Mar 27 2017, 3:25 PM
anemet added inline comments.
include/llvm/IR/Operator.h
177–178 ↗(On Diff #92521)

Yes, that is why I added the comment. You're right I mixed up the name of the field.

209–211 ↗(On Diff #92521)

Setting it with false is used in the clang patch in https://reviews.llvm.org/D31168.

But yes, I am planning to change all of these as a follow-on. I was going to put it in this patchset but the change is actually larger than I expected.

anemet updated this revision to Diff 93187.Mar 27 2017, 3:31 PM
anemet marked an inline comment as done.

Address Sanjay's comments

spatel accepted this revision.Mar 27 2017, 4:10 PM

Thanks! LGTM.

This revision is now accepted and ready to land.Mar 27 2017, 4:10 PM
This revision was automatically updated to reflect the committed changes.