This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Make sure FMF are propagated when getSetcc canonicalizes FP constants to RHS.
ClosedPublic

Authored by craig.topper on Sep 21 2020, 6:52 PM.

Details

Summary

getNode handling for ISD:SETCC calls FoldSETCC which can canonicalize
FP constants to the RHS. When this happens we should create the node
with the FMF that was requested. By using FlagInserter when can ensure
any calls to getNode/getSetcc during canonicalization will also get the flags.

Diff Detail

Event Timeline

craig.topper created this revision.Sep 21 2020, 6:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2020, 6:52 PM
craig.topper requested review of this revision.Sep 21 2020, 6:52 PM

What, if any, interaction is there between this and D87361?
Can we solve this with the more general flag propagation scheme proposed there?

qiucf added a comment.Sep 22 2020, 8:54 AM

What, if any, interaction is there between this and D87361?
Can we solve this with the more general flag propagation scheme proposed there?

I think yes, but here the issue is getSetCC uses a default empty argument for Flags, just like how getNode do. If split it into two versions like D87361, one as normal but Flags is explicitly required, the other for no Flags and use the flags from current inserter (or remove Flags argument, if provide setFlags to flags inserter for rare cases when we want to pass a different flag), we can do it.

Use the FlagInserter mechanism in SelectionDAGBuilder::visitFCmp.
Remove Flags from getSetcc which was previously added for SelectionDAGBuilder.

craig.topper edited the summary of this revision. (Show Details)Sep 26 2020, 9:43 PM
spatel accepted this revision.Sep 29 2020, 9:00 AM

LGTM - note that having FMF on fcmp in IR is an open problem (https://llvm.org/PR38086), so having it on setcc is probably the same. But this patch doesn't make that any worse AFAICT.

This revision is now accepted and ready to land.Sep 29 2020, 9:00 AM
This revision was landed with ongoing or failed builds.Oct 5 2020, 3:02 PM
This revision was automatically updated to reflect the committed changes.