Page MenuHomePhabricator

[SDAG] Add AllowContract to SNodeFlags
ClosedPublic

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

Details

Summary

Properly propagate the FMF from the LLVM IR to this flag.

This is toward moving fp-contraction=fast from an LLVM TargetOption to a
FastMathFlag in order to fix PR25721.

Diff Detail

Repository
rL LLVM

Event Timeline

anemet created this revision.Mar 20 2017, 8:09 PM
anemet added subscribers: spatel, arsenm.

@spatel, @arsenm, this is a trivial patch to expose the new FMF to SDAG. Since you guys looked at the rest of the LLVM patches in the set, would you mind reviewing this too?

spatel accepted this revision.Mar 28 2017, 3:03 PM

@spatel, @arsenm, this is a trivial patch to expose the new FMF to SDAG. Since you guys looked at the rest of the LLVM patches in the set, would you mind reviewing this too?

I actually haven't looked at all of the patches. Can you add me to those?

This patch LGTM. I made a couple of small suggestions for follow-ups.

I don't know if this is addressed in one of the other patches, but we'll want to extend FMF to more than binops...or we won't be able to propagate the contract flag to FMA nodes.

include/llvm/CodeGen/SelectionDAGNodes.h
396 ↗(On Diff #92421)

VectorReduction is missing. It's an independent change from this patch of course, but since you're here...

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2589 ↗(On Diff #92421)

I hid these behind a flag, so it would be easier to test, but it should be safe to remove this now. It's an independent change from this patch of course, but since you're here... :)

This revision is now accepted and ready to land.Mar 28 2017, 3:03 PM
This revision was automatically updated to reflect the committed changes.
anemet marked 2 inline comments as done.

@spatel, @arsenm, this is a trivial patch to expose the new FMF to SDAG. Since you guys looked at the rest of the LLVM patches in the set, would you mind reviewing this too?

I actually haven't looked at all of the patches. Can you add me to those?

You were already added to the LLVM patches (there is one left for now but I am working on more ;). I'll add you to the clang ones too.

This patch LGTM. I made a couple of small suggestions for follow-ups.

Thanks, Sanjay! I committed the changes you requested (r298962, r298963).

I don't know if this is addressed in one of the other patches, but we'll want to extend FMF to more than binops...or we won't be able to propagate the contract flag to FMA nodes.

Not, it's not addressed; we can do that later if it's useful. When is that useful?

I don't know if this is addressed in one of the other patches, but we'll want to extend FMF to more than binops...or we won't be able to propagate the contract flag to FMA nodes.

Not, it's not addressed; we can do that later if it's useful. When is that useful?

I'm not sure, but the examples in D31164 ( x + x + x -> 3*x ) made me think we're probably missing some folds that could involve an FMA. It definitely would help when fast/reassociate/unsafe are in play as we saw in D28675.