This is an archive of the discontinued LLVM Phabricator instance.

[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

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

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

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

2199

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

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

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

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

Do you have an example of such transformations?

mehdi_amini added inline comments.Mar 20 2017, 9:10 PM
docs/LangRef.rst
2199

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

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

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

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

@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
175–176

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.

207–209

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.

317–318

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
175–176

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

207–209

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.