[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

1116

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/include/llvm/CodeGen/SelectionDAG.h

#endif
   ///
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
                   ArrayRef<SDUse> Ops);
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
                   ArrayRef<SDValue> Ops, const SDNodeFlags Flags = SDNodeFlags());
   SDValue getNode(unsigned Opcode, const SDLoc &DL, ArrayRef<EVT> ResultTys,
                   ArrayRef<SDValue> Ops);
   SDValue getNode(unsigned Opcode, const SDLoc &DL, SDVTList VTList,
-                  ArrayRef<SDValue> Ops);
+                  ArrayRef<SDValue> Ops, const SDNodeFlags Flags = SDNodeFlags());
 
   // Specialize based on number of operands.
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT);
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT, SDValue Operand,
                   const SDNodeFlags Flags = SDNodeFlags());
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT, SDValue N1,
                   SDValue N2, const SDNodeFlags Flags = SDNodeFlags());
   SDValue getNode(unsigned Opcode, const SDLoc &DL, EVT VT, SDValue N1,

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 }
 
 SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL,
                               ArrayRef<EVT> ResultTys, ArrayRef<SDValue> Ops) {
   return getNode(Opcode, DL, getVTList(ResultTys), Ops);
 }
 
 SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, SDVTList VTList,
-                              ArrayRef<SDValue> Ops) {
+                              ArrayRef<SDValue> Ops, const SDNodeFlags Flags) {
   if (VTList.NumVTs == 1)
     return getNode(Opcode, DL, VTList.VTs[0], Ops);
 
   switch (Opcode) {
   case ISD::STRICT_FP_EXTEND:
     assert(VTList.NumVTs == 2 && Ops.size() == 2 &&
            "Invalid STRICT_FP_EXTEND!");
     assert(VTList.VTs[0].isFloatingPoint() &&
#endif
   if (VTList.VTs[VTList.NumVTs-1] != MVT::Glue) {
     FoldingSetNodeID ID;
     AddNodeIDNode(ID, Opcode, VTList, Ops);
     void *IP = nullptr;
     if (SDNode *E = FindNodeOrInsertPos(ID, DL, IP))
       return SDValue(E, 0);
 
     N = newSDNode<SDNode>(Opcode, DL.getIROrder(), DL.getDebugLoc(), VTList);
+    N->setFlags(Flags);
     createOperands(N, Ops);
     CSEMap.InsertNode(N, IP);
   } else {
     N = newSDNode<SDNode>(Opcode, DL.getIROrder(), DL.getDebugLoc(), VTList);
     createOperands(N, Ops);
   }
   InsertNode(N);
   SDValue V(N, 0);

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visit(const Instruction &I) {
   if (!isa<DbgInfoIntrinsic>(I))
     ++SDNodeOrder;
 
   CurInst = &I;
 
   visit(I.getOpcode(), I);
 
   if (auto *FPMO = dyn_cast<FPMathOperator>(&I)) {
+    // ContrainedFPIntrinsics handle their own FMF.
+    if (!isa<ConstrainedFPIntrinsic>(&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);
         if (!Node->getFlags().isDefined())
           Node->setFlags(IncomingFlags);
         else
           Node->intersectFlagsWith(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);
-      }
 
   if (!I.isTerminator() && !HasTailCall &&
       !isStatepoint(&I)) // statepoints handle their exports internally
     CopyToExportRegsIfNeeded(&I);
 
   CurInst = nullptr;
 }
 
case fp::ExceptionBehavior::ebStrict:
       PendingConstrainedFPStrict.push_back(OutChain);
       break;
     }
   };
 
   SDVTList VTs = DAG.getVTList(ValueVTs);
   fp::ExceptionBehavior EB = FPI.getExceptionBehavior().getValue();
 
+  SDNodeFlags Flags;
+  if (EB == fp::ExceptionBehavior::ebIgnore)
+    Flags.setNoFPExcept(true);
+
+  if (auto *FPOp = dyn_cast<FPMathOperator>(&FPI))
+    Flags.copyFMF(*FPOp);
+
   unsigned Opcode;
   switch (FPI.getIntrinsicID()) {
   default: llvm_unreachable("Impossible intrinsic");  // Can't reach here.
 #define DAG_INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC, DAGN)               \
   case Intrinsic::INTRINSIC:                                                   \
     Opcode = ISD::STRICT_##DAGN;                                               \
     break;
 #include "llvm/IR/ConstrainedOps.def"
   case Intrinsic::experimental_constrained_fmuladd: {
     Opcode = ISD::STRICT_FMA;
     // Break fmuladd into fmul and fadd.
     if (TM.Options.AllowFPOpFusion == FPOpFusion::Strict ||
         !TLI.isFMAFasterThanFMulAndFAdd(DAG.getMachineFunction(),
                                         ValueVTs[0])) {
       Opers.pop_back();
-      SDValue Mul = DAG.getNode(ISD::STRICT_FMUL, sdl, VTs, Opers);
+      SDValue Mul = DAG.getNode(ISD::STRICT_FMUL, sdl, VTs, Opers, Flags);
       pushOutChain(Mul, EB);
       Opcode = ISD::STRICT_FADD;
       Opers.clear();
       Opers.push_back(Mul.getValue(1));
       Opers.push_back(Mul.getValue(0));
       Opers.push_back(getValue(FPI.getArgOperand(2)));
     }
     break;
#include "llvm/IR/ConstrainedOps.def"
   case ISD::STRICT_FSETCC:
   case ISD::STRICT_FSETCCS: {
     auto *FPCmp = dyn_cast<ConstrainedFPCmpIntrinsic>(&FPI);
     Opers.push_back(DAG.getCondCode(getFCmpCondCode(FPCmp->getPredicate())));
     break;
   }
   }
 
-  SDValue Result = DAG.getNode(Opcode, sdl, VTs, Opers);
+  SDValue Result = DAG.getNode(Opcode, sdl, VTs, Opers, Flags);
   pushOutChain(Result, EB);
 
   SDValue FPResult = Result.getValue(0);
   setValue(&FPI, FPResult);
 }
 
 std::pair<SDValue, SDValue>
 SelectionDAGBuilder::lowerInvokable(TargetLowering::CallLoweringInfo &CLI,

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