Page MenuHomePhabricator

[IR] allow fast-math-flags on phi of FP values
ClosedPublic

Authored by spatel on Sep 13 2019, 10:36 AM.

Details

Summary

The changes here are based on the corresponding diffs for allowing FMF on 'select':
D61917

As discussed there, we want to have fast-math-flags be a property of an FP value because the alternative (having them on things like fcmp) leads to logical inconsistency such as:
https://bugs.llvm.org/show_bug.cgi?id=38086

The earlier patch for select made almost no practical difference because most unoptimized conditional code begins life as a phi (based on what I see in clang).
Similarly, I don't expect this patch to do much on its own either because SimplifyCFG promptly drops the flags when converting to select on a minimal example like:
https://bugs.llvm.org/show_bug.cgi?id=39535

But once we have this plumbing in place, we should be able to wire up the FMF propagation and start solving cases like that.

The change to RecurrenceDescriptor::AddReductionVar() is required to prevent a regression in a LoopVectorize test. We are intersecting the FMF of any FPMathOperator there, so if a phi is not properly annotated, new math instructions may not be either. Once we fix the propagation in SimplifyCFG, it may be safe to remove that hack.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Sep 13 2019, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 10:36 AM

Maybe a dumb question, but what happens if two incoming PHI values have different FMFs set? Does the PHI's FMF override those? If so, is that safe?

WRT divergent phi FMF, if you wanted to be pessimistic you could do an intersection(and), doing an inclusion(or) might include too much. Either way behavior would change. The intersection already has other prior context in FMF.

WRT divergent phi FMF, if you wanted to be pessimistic you could do an intersection(and), doing an inclusion(or) might include too much. Either way behavior would change. The intersection already has other prior context in FMF.

Right - we have places (like the IVDescriptors diff in this patch) that intersects FMF. Most IR has uniform FMF because it's set in the front-end and passed as-is through the optimizer. Ie, it takes an LTO scenario (with differing FP optimization flags on compiled modules) to see non-uniform FMF, so it's not that common (yet?).

I don't know of any places in the IR optimizer currently where we'd get this wrong by being too liberal. It's more likely -- as with the SimplifyCFG transform mentioned in the description -- that we lose FMF completely.

cameron.mcinally accepted this revision.Mon, Sep 16, 8:20 AM

LGTM

My confidence is somewhat low though...

This revision is now accepted and ready to land.Mon, Sep 16, 8:20 AM

LGTM

Thanks!

My confidence is somewhat low though...

Sure, can someone else please take a look? (adding a few more potential reviewers from the earlier patch)

Ping - anyone want to issue a 2nd opinion/review?

I think this makes sense since select already has those attrs.
But what should happen for non-fp phi's? Do those flags make sense on e.g. i32 ?

I think this makes sense since select already has those attrs.
But what should happen for non-fp phi's? Do those flags make sense on e.g. i32 ?

Unless the definition of FPMathOperator is broken, we won't allow FMF on a non-FP type of phi/select.
Or are you asking if things like nsw/nuw/exact make sense on a phi with integer type? I haven't thought about that...
(We can't have both FMF and wrapping/exact flags simultaneously because the underlying implementation shares the bits of SubclassOptionalData.)

I think this makes sense since select already has those attrs.
But what should happen for non-fp phi's? Do those flags make sense on e.g. i32 ?

Unless the definition of FPMathOperator is broken, we won't allow FMF on a non-FP type of phi/select.

Yes, that was the question.

Or are you asking if things like nsw/nuw/exact make sense on a phi with integer type? I haven't thought about that...

No, that doesn't make sense. Those only make sense on actual instructions.

(We can't have both FMF and wrapping/exact flags simultaneously because the underlying implementation shares the bits of SubclassOptionalData.)

jmolloy accepted this revision.Tue, Sep 24, 11:01 AM

LGTM, sorry for the long latency.

This revision was automatically updated to reflect the committed changes.