This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAGBuilder] Pass fast math flags to getNode calls rather than trying to set them after the fact.
ClosedPublic

Authored by craig.topper on Sep 5 2020, 8:06 PM.
Tokens
"Orange Medal" token, awarded by post.kadirselcuk.

Details

Summary

This removes the after the fact FMF handling from D46854 in favor of passing fast math flags to getNode. This should be a superset of D87130.

Diff Detail

Event Timeline

craig.topper created this revision.Sep 5 2020, 8:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2020, 8:06 PM
craig.topper requested review of this revision.Sep 5 2020, 8:06 PM
nikic added a subscriber: nikic.Sep 6 2020, 12:38 AM

I drafted a similar patch, and I think removing the saved-state complexity is worth it.
The other possibility -- and this patch doesn't have to be gated on this, but it could save a lot of back-and-forth -- is to set up a FastMathFlagGuard object like we have in IRBuilder (adding reviewers from D87037).

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4875–4877

Should use the incoming Flags now.

4889

Should use the incoming Flags on all created nodes now.

5010

Should use the incoming Flags on all created nodes now.

5085

Should use the incoming Flags on all created nodes now.

5197

Should use the incoming Flags on all created nodes now.

8956

Could get rid of the FMF temp and use SDFlags everywhere below.

llvm/test/CodeGen/PowerPC/fmf-propagation.ll
560

We need to find where this flag got dropped?

craig.topper added inline comments.Sep 6 2020, 9:13 AM
llvm/test/CodeGen/PowerPC/fmf-propagation.ll
560

It was getting a FMF flag before because it was the last instruction created by handling a call. So the after the fact handling found it. I can’t think of a reason we need FMF on a copy. If we need them anywhere it would be on the CALL_NOP right?

craig.topper added inline comments.Sep 6 2020, 11:20 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5010

Can we do that in a follow up? There's a quite a bit of code in these approximations. Adding the flags could enable FMA formation or reassociation that we weren't doing before.

spatel accepted this revision.Sep 7 2020, 6:18 AM

LGTM, but please wait at least a day to commit in case there are other comments.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5010

Yes - but on 2nd thought, we probably shouldn't do that on any of these expansions. IIRC, we had to undo the FMF propagation on some of the x86 cast expansions to avoid bungling the expected bit-hacks. We should just assume these expansions are the best as-is.

llvm/test/CodeGen/PowerPC/fmf-propagation.ll
560

Yes, that makes sense. If we need a flag, it should be on the call, so that's independent of this patch.

This revision is now accepted and ready to land.Sep 7 2020, 6:18 AM
spatel added inline comments.Sep 7 2020, 6:21 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5010

Could still update the TODO comments though - ie, we are now intentionally not propagating FMF because these are hand-crafted expansions, and we don't expect the code to be altered further.

This revision was landed with ongoing or failed builds.Sep 8 2020, 3:28 PM
This revision was automatically updated to reflect the committed changes.