This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG][FPEnv] Take into account SelectionDAG continuous CSE when setting the nofpexcept flag for constrained intrinsics
ClosedPublic

Authored by craig.topper on Feb 26 2020, 6:24 PM.

Details

Summary

SelectionDAG CSEs nodes based on their result type and operands, but not their flags. The flags are expected to be intersected when they are CSEd. For FP nodes we manage both the fast math flags and the nofpexcept flag after the nodes have already been CSEd when they were created with getNode. The fast math flags we are managing correctly, but similar was not done for the nofpexcept flag.

Unfortunately, fixing this is a little involved because most constrained intrinsics are considered FPMathOperators, but not all. The check is based on whether they return an FP type or not. These nodes can have fast math flags. In order for the CSE detection to work we need to handle the fast math flags for the FPMathOperators and the nofpexcept flag for the constrained intrinsics that are FPMathOperators at the same time. This way there is only one call to set the flags per newly created node. So we need a separate block for the remaining intrinsics that aren't FPMathOperators.

We should probably make FPMathOperators also include all constrained intrinsics. But that requires making Operator.h include the intrinsic enums or IntrinsicInst.h which may expose other issues.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 26 2020, 6:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2020, 6:24 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
pengfei added inline comments.Feb 26 2020, 7:57 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1137

Can we just use below code here:

SDNodeFlags IncomingFlags;
IncomingFlags.setNoFPExcept(true);
if (!Node->getFlags().isDefined())
  Node->setFlags(IncomingFlags);
else
  Node->intersectFlagsWith(IncomingFlags);

Simpler version to think about. Just let Constrained intrinsics handle their own flags completely, and skip them in the common FMF handling.

@spatel what do you think?

Simpler version to think about. Just let Constrained intrinsics handle their own flags completely, and skip them in the common FMF handling.

@spatel what do you think?

Seems fine to me. Could we do something similar for reductions since we can mistakenly drop flags on those too?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1151–1152

typo - 'Constrained'

Fix typo in comment.

For reductions, do you think we can just check that the reduction flag is already set? Or do we need to just excluse all BinaryOperators from the geneirc handler and just fully manage FMF for BinaryOperators in its visitor function?

spatel accepted this revision.Mar 18 2020, 12:38 PM

Fix typo in comment.

For reductions, do you think we can just check that the reduction flag is already set? Or do we need to just excluse all BinaryOperators from the geneirc handler and just fully manage FMF for BinaryOperators in its visitor function?

Just checking the flag seems like it should work, but I haven't stepped through a reduction example, so I'm not sure.
Either way, let's push this change and see if it uncovers any other flag problems. LGTM.

This revision is now accepted and ready to land.Mar 18 2020, 12:38 PM
This revision was automatically updated to reflect the committed changes.