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

[craig.topper]

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.

[pengfei]

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);

[craig.topper]

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?

[spatel]

In D75224#1927019, @craig.topper wrote:

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'

[craig.topper]

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]

In D75224#1927879, @craig.topper wrote:

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.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (auto *FPMO = dyn_cast<FPMathOperator>(&I)) {
     // Propagate the fast-math-flags of this IR instruction to the DAG node that
     // maps to this instruction.
     // TODO: We could handle all flags (nsw, etc) here.
     // TODO: If an IR instruction maps to >1 node, only the final node will have
     //       flags set.
     if (SDNode *Node = getNodeForIRValue(&I)) {
       SDNodeFlags IncomingFlags;
       IncomingFlags.copyFMF(*FPMO);
+
+      // Constrained FP intrinsics with fpexcept.ignore should also get
+      // the NoFPExcept flag.
+      if (auto *FPI = dyn_cast<ConstrainedFPIntrinsic>(&I))
+        if (FPI->getExceptionBehavior() == fp::ExceptionBehavior::ebIgnore)
+          IncomingFlags.setNoFPExcept(true);
+
       if (!Node->getFlags().isDefined())
         Node->setFlags(IncomingFlags);
       else
         Node->intersectFlagsWith(IncomingFlags);
     }
-  }
+  } else if (auto *FPI = dyn_cast<ConstrainedFPIntrinsic>(&I)) {
+    // FIXME: Not every ConstrainedFPIntrinsic is a FPMathOperator.
+    if (SDNode *Node = getNodeForIRValue(&I)) {
+      SDNodeFlags IncomingFlags;
+
       // Constrained FP intrinsics with fpexcept.ignore should also get
       // the NoFPExcept flag.
-  if (auto *FPI = dyn_cast<ConstrainedFPIntrinsic>(&I))
       if (FPI->getExceptionBehavior() == fp::ExceptionBehavior::ebIgnore)
-      if (SDNode *Node = getNodeForIRValue(&I)) {
-        SDNodeFlags Flags = Node->getFlags();
-        Flags.setNoFPExcept(true);
-        Node->setFlags(Flags);
-      }
+        IncomingFlags.setNoFPExcept(true);
+
+      if (!Node->getFlags().isDefined())
+        Node->setFlags(IncomingFlags);
+      else
+        Node->intersectFlagsWith(IncomingFlags);
+    }
 
   if (!I.isTerminator() && !HasTailCall &&
       !isStatepoint(&I)) // statepoints handle their exports internally
     CopyToExportRegsIfNeeded(&I);
 
   CurInst = nullptr;
 }
 
 void SelectionDAGBuilder::visitPHI(const PHINode &) {

llvm/test/CodeGen/X86/fp-intrinsics-flags.ll

 ; These 2 divs only differ in their exception behavior and will be CSEd. Make
 ; sure the nofpexcept flag is not set on the combined node.
 define void @binop_cse(double %a, double %b, double* %x, double* %y) #0 {
 entry:
 ; CHECK-LABEL: name: binop_cse
 ; CHECK: [[MOV32rm:%[0-9]+]]:gr32 = MOV32rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.0)
 ; CHECK: [[MOV32rm1:%[0-9]+]]:gr32 = MOV32rm %fixed-stack.1, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.1, align 16)
 ; CHECK: [[MOVSDrm_alt:%[0-9]+]]:fr64 = MOVSDrm_alt %fixed-stack.3, 1, $noreg, 0, $noreg :: (load 8 from %fixed-stack.3, align 16)
-; CHECK: %3:fr64 = nofpexcept DIVSDrm [[MOVSDrm_alt]], %fixed-stack.2, 1, $noreg, 0, $noreg, implicit $mxcsr :: (load 8 from %fixed-stack.2)
+; CHECK: %3:fr64 = DIVSDrm [[MOVSDrm_alt]], %fixed-stack.2, 1, $noreg, 0, $noreg, implicit $mxcsr :: (load 8 from %fixed-stack.2)
 ; CHECK: MOVSDmr killed [[MOV32rm1]], 1, $noreg, 0, $noreg, %3 :: (store 8 into %ir.x, align 4)
 ; CHECK: MOVSDmr killed [[MOV32rm]], 1, $noreg, 0, $noreg, %3 :: (store 8 into %ir.y, align 4)
 ; CHECK: RET 0
   %div = call double @llvm.experimental.constrained.fdiv.f64(double %a, double %b, metadata !"round.dynamic", metadata !"fpexcept.strict") #0
   %div2 = call double @llvm.experimental.constrained.fdiv.f64(double %a, double %b, metadata !"round.dynamic", metadata !"fpexcept.ignore") #0
   store double %div, double* %x
   store double %div2, double* %y
   ret void
 }
 
 ; These 2 sitofps only differ in their exception behavior and will be CSEd. Make
 ; sure the nofpexcept flag is not set on the combined node.
 define void @sitofp_cse(i32 %a, double* %x, double* %y) #0 {
 entry:
 ; CHECK-LABEL: name: sitofp_cse
 ; CHECK: [[MOV32rm:%[0-9]+]]:gr32 = MOV32rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.0, align 8)
 ; CHECK: [[MOV32rm1:%[0-9]+]]:gr32 = MOV32rm %fixed-stack.1, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.1)
-; CHECK: %2:fr64 = nofpexcept CVTSI2SDrm %fixed-stack.2, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.2, align 16)
+; CHECK: %2:fr64 = CVTSI2SDrm %fixed-stack.2, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.2, align 16)
 ; CHECK: MOVSDmr killed [[MOV32rm1]], 1, $noreg, 0, $noreg, %2 :: (store 8 into %ir.x, align 4)
 ; CHECK: MOVSDmr killed [[MOV32rm]], 1, $noreg, 0, $noreg, %2 :: (store 8 into %ir.y, align 4)
 ; CHECK: RET 0
   %result = call double @llvm.experimental.constrained.sitofp.f64.i32(i32 %a, metadata !"round.dynamic", metadata !"fpexcept.strict") #0
   %result2 = call double @llvm.experimental.constrained.sitofp.f64.i32(i32 %a, metadata !"round.dynamic", metadata !"fpexcept.ignore") #0
   store double %result, double* %x
   store double %result2, double* %y
   ret void