Address review comments

spavloff · spavloff · commit 6149fa1b0eef · 2025-11-01T20:36:57.000+07:00
- Add IRBuilder::resetModeToStrictFP instead of adding extra argument to
  setIsFPConstrained.
- Change wording in the Release Notes.
- Modify printing MemoryEffects of FP operation.
- Fix IRBuilder::CreateCall.
- Clean up unit test.
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1089,7 +1089,7 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
   if ((FD && (FD->UsesFPIntrin() || FD->hasAttr<StrictFPAttr>())) ||
       (!FD && (FPExceptionBehavior != llvm::fp::ebIgnore ||
                RM != llvm::RoundingMode::NearestTiesToEven))) {
-    Builder.setIsFPConstrained(true, false);
+    Builder.setIsFPConstrained(true);
     Fn->addFnAttr(llvm::Attribute::StrictFP);
   }
 
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
@@ -68,6 +68,8 @@ Changes to the LLVM IR
   pointers) argument.
 * A `load atomic` may now be used with vector types on x86.
 * Floating-point operand bundles have been added.
+* Calls to floating-point intrinsics can have operand bundles "fp.round" and
+  "fp.except", which specify effective rounding mode and exception behavior.
 
 Changes to LLVM infrastructure
 ------------------------------
diff --git a/llvm/include/llvm/IR/FPEnv.h b/llvm/include/llvm/IR/FPEnv.h
@@ -81,6 +81,11 @@ LLVM_ABI std::optional<StringRef>
 std::optional<StringRef>
     convertExceptionBehaviorToBundle(fp::ExceptionBehavior);
 
+inline raw_ostream &operator<<(raw_ostream &OS, fp::ExceptionBehavior EB) {
+  OS << convertExceptionBehaviorToBundle(EB).value_or("invalid");
+  return OS;
+}
+
 /// Returns true if the exception handling behavior and rounding mode
 /// match what is used in the default floating point environment.
 inline bool isDefaultFPEnvironment(fp::ExceptionBehavior EB, RoundingMode RM) {
diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h
@@ -348,17 +348,19 @@ class IRBuilderBase {
   /// enabled the CreateF<op>() calls instead create constrained
   /// floating point intrinsic calls. Fast math flags are unaffected
   /// by this setting.
-  void setIsFPConstrained(bool IsCon, bool AndReset = true) {
-    if (AndReset) {
-      if (IsCon) {
-        setDefaultConstrainedRounding(RoundingMode::Dynamic);
-        setDefaultConstrainedExcept(fp::ebStrict);
-      } else {
-        setDefaultConstrainedRounding(RoundingMode::NearestTiesToEven);
-        setDefaultConstrainedExcept(fp::ebIgnore);
-      }
+  void setIsFPConstrained(bool IsCon) { IsFPConstrained = IsCon; }
+
+  /// Enable/Disable use of constrained floating point math and reset FP options
+  /// according to the selected mode.
+  void resetModeToStrictFP(bool IsCon) {
+    if (IsCon) {
+      setDefaultConstrainedRounding(RoundingMode::Dynamic);
+      setDefaultConstrainedExcept(fp::ebStrict);
+    } else {
+      setDefaultConstrainedRounding(RoundingMode::NearestTiesToEven);
+      setDefaultConstrainedExcept(fp::ebIgnore);
     }
-    IsFPConstrained = IsCon;
+    setIsFPConstrained(IsCon);
   }
 
   /// Query for the use of constrained floating point math
diff --git a/llvm/lib/CodeGen/AtomicExpandPass.cpp b/llvm/lib/CodeGen/AtomicExpandPass.cpp
@@ -159,7 +159,7 @@ struct ReplacementIRBuilder
     SetInsertPoint(I);
     this->CollectMetadataToCopy(I, {LLVMContext::MD_pcsections});
     if (BB->getParent()->getAttributes().hasFnAttr(Attribute::StrictFP))
-      this->setIsFPConstrained(true);
+      this->resetModeToStrictFP(true);
 
     MMRAMD = I->getMetadata(LLVMContext::MD_mmra);
   }
@@ -1716,7 +1716,7 @@ bool AtomicExpandImpl::tryExpandAtomicCmpXchg(AtomicCmpXchgInst *CI) {
 bool llvm::expandAtomicRMWToCmpXchg(AtomicRMWInst *AI,
                                     CreateCmpXchgInstFun CreateCmpXchg) {
   ReplacementIRBuilder Builder(AI, AI->getDataLayout());
-  Builder.setIsFPConstrained(
+  Builder.resetModeToStrictFP(
       AI->getFunction()->hasFnAttribute(Attribute::StrictFP));
 
   // FIXME: If FP exceptions are observable, we should force them off for the
diff --git a/llvm/lib/CodeGen/HardwareLoops.cpp b/llvm/lib/CodeGen/HardwareLoops.cpp
@@ -503,7 +503,7 @@ Value *HardwareLoop::InitLoopCount() {
 Value* HardwareLoop::InsertIterationSetup(Value *LoopCountInit) {
   IRBuilder<> Builder(BeginBB->getTerminator());
   if (BeginBB->getParent()->getAttributes().hasFnAttr(Attribute::StrictFP))
-    Builder.setIsFPConstrained(true);
+    Builder.resetModeToStrictFP(true);
   Type *Ty = LoopCountInit->getType();
   bool UsePhi = UsePHICounter || Opts.ForcePhi;
   Intrinsic::ID ID = UseLoopGuard
@@ -537,7 +537,7 @@ void HardwareLoop::InsertLoopDec() {
   IRBuilder<> CondBuilder(ExitBranch);
   if (ExitBranch->getParent()->getParent()->getAttributes().hasFnAttr(
           Attribute::StrictFP))
-    CondBuilder.setIsFPConstrained(true);
+    CondBuilder.resetModeToStrictFP(true);
 
   Value *Ops[] = { LoopDecrement };
   Value *NewCond = CondBuilder.CreateIntrinsic(Intrinsic::loop_decrement,
@@ -560,7 +560,7 @@ Instruction* HardwareLoop::InsertLoopRegDec(Value *EltsRem) {
   IRBuilder<> CondBuilder(ExitBranch);
   if (ExitBranch->getParent()->getParent()->getAttributes().hasFnAttr(
           Attribute::StrictFP))
-    CondBuilder.setIsFPConstrained(true);
+    CondBuilder.resetModeToStrictFP(true);
 
   Value *Ops[] = { EltsRem, LoopDecrement };
   Value *Call = CondBuilder.CreateIntrinsic(Intrinsic::loop_decrement_reg,
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -8360,33 +8360,6 @@ void SelectionDAGBuilder::pushFPOpOutChain(SDValue Result,
   }
 }
 
-void SelectionDAGBuilder::pushOutChain(SDValue Result,
-                                       fp::ExceptionBehavior EB) {
-  assert(Result.getNode()->getNumValues() == 2);
-
-  // Push node to the appropriate list so that future instructions can be
-  // chained up correctly.
-  SDValue OutChain = Result.getValue(1);
-  switch (EB) {
-  case fp::ExceptionBehavior::ebIgnore:
-    // The only reason why ebIgnore nodes still need to be chained is that
-    // they might depend on the current rounding mode, and therefore must
-    // not be moved across instruction that may change that mode.
-    [[fallthrough]];
-  case fp::ExceptionBehavior::ebMayTrap:
-    // These must not be moved across calls or instructions that may change
-    // floating-point exception masks.
-    PendingConstrainedFP.push_back(OutChain);
-    break;
-  case fp::ExceptionBehavior::ebStrict:
-    // These must not be moved across calls or instructions that may change
-    // floating-point exception masks or read floating-point exception flags.
-    // In addition, they cannot be optimized out even if unused.
-    PendingConstrainedFPStrict.push_back(OutChain);
-    break;
-  }
-}
-
 void SelectionDAGBuilder::visitConstrainedFPIntrinsic(
     const ConstrainedFPIntrinsic &FPI) {
   SDLoc sdl = getCurSDLoc();
@@ -9496,7 +9469,7 @@ bool SelectionDAGBuilder::visitFPOperation(const CallInst &I, unsigned Opcode) {
   SDLoc sdl = getCurSDLoc();
   SDValue Result = DAG.getNode(Opcode, sdl, NodeVT, Operands, Flags);
   if (HasChain)
-    pushOutChain(Result, EB);
+    pushFPOpOutChain(Result, EB);
 
   SDValue FPResult = Result.getValue(0);
   setValue(&I, FPResult);
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
@@ -379,8 +379,6 @@ class SelectionDAGBuilder {
   /// Evict any dangling debug information, attempting to salvage it first.
   void resolveOrClearDbgInfo();
 
-  void pushOutChain(SDValue Result, fp::ExceptionBehavior EB);
-
   SDValue getValue(const Value *V);
 
   SDValue getNonRegisterValue(const Value *V);
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
@@ -106,6 +106,10 @@ static cl::opt<bool> PreserveAssemblyUseListOrder(
     "preserve-ll-uselistorder", cl::Hidden, cl::init(false),
     cl::desc("Preserve use-list order when writing LLVM assembly."));
 
+static cl::opt<bool> PrintFPMemoryEffects(
+    "print-fp-memory-effects", cl::Hidden,
+    cl::desc("Pretty print floating-point memory effects when dumping"));
+
 // Make virtual table appear in this compilation unit.
 AssemblyAnnotationWriter::~AssemblyAnnotationWriter() = default;
 
@@ -4321,6 +4325,25 @@ void AssemblyWriter::printGCRelocateComment(const GCRelocateInst &Relocate) {
   Out << ")";
 }
 
+static void printFPMemoryEffects(raw_ostream &Out, MemoryEffects ME) {
+  ModRefInfo MR = ME.getModRef(IRMemLocation::InaccessibleMem);
+  Out << " ; fpe=[";
+  switch (MR) {
+  case ModRefInfo::NoModRef:
+    break;
+  case ModRefInfo::Ref:
+    Out << "r";
+    break;
+  case ModRefInfo::Mod:
+    Out << "w";
+    break;
+  case ModRefInfo::ModRef:
+    Out << "rw";
+    break;
+  }
+  Out << "]";
+}
+
 /// printInfoComment - Print a little comment after the instruction indicating
 /// which slot it occupies.
 void AssemblyWriter::printInfoComment(const Value &V) {
@@ -4351,30 +4374,14 @@ void AssemblyWriter::printInfoComment(const Value &V) {
   if (PrintInstAddrs)
     Out << " ; " << &V;
 
-  if (auto *CI = dyn_cast<CallInst>(&V))
-    if (Intrinsic::ID IID = CI->getIntrinsicID())
-      if (IntrinsicInst::isFloatingPointOperation(IID))
-        if (const BasicBlock *BB = CI->getParent())
-          if (const Function *F = BB->getParent())
-            if (F->hasFnAttribute(Attribute::StrictFP)) {
-              MemoryEffects ME = CI->getMemoryEffects();
-              ModRefInfo MR = ME.getModRef(IRMemLocation::InaccessibleMem);
-              Out << " ; fpe=[";
-              switch (MR) {
-              case ModRefInfo::NoModRef:
-                break;
-              case ModRefInfo::Ref:
-                Out << "r";
-                break;
-              case ModRefInfo::Mod:
-                Out << "w";
-                break;
-              case ModRefInfo::ModRef:
-                Out << "rw";
-                break;
-              }
-              Out << "]";
-            }
+  if (PrintFPMemoryEffects) {
+    if (auto *CI = dyn_cast<CallInst>(&V))
+      if (Intrinsic::ID IID = CI->getIntrinsicID())
+        if (const Function *F = CI->getFunction())
+          if (IntrinsicInst::isFloatingPointOperation(IID) &&
+              F->hasFnAttribute(Attribute::StrictFP))
+            printFPMemoryEffects(Out, CI->getMemoryEffects());
+  }
 }
 
 static void maybePrintCallAddrSpace(const Value *Operand, const Instruction *I,
diff --git a/llvm/lib/IR/FPEnv.cpp b/llvm/lib/IR/FPEnv.cpp
@@ -76,27 +76,20 @@ std::optional<StringRef> convertRoundingModeToBundle(RoundingMode UseRounding) {
   std::optional<StringRef> RoundingStr;
   switch (UseRounding) {
   case RoundingMode::Dynamic:
-    RoundingStr = "dynamic";
-    break;
+    return "dynamic";
   case RoundingMode::NearestTiesToEven:
-    RoundingStr = "tonearest";
-    break;
+    return "tonearest";
   case RoundingMode::NearestTiesToAway:
-    RoundingStr = "tonearestaway";
-    break;
+    return "tonearestaway";
   case RoundingMode::TowardNegative:
-    RoundingStr = "downward";
-    break;
+    return "downward";
   case RoundingMode::TowardPositive:
-    RoundingStr = "upward";
-    break;
+    return "upward";
   case RoundingMode::TowardZero:
-    RoundingStr = "towardzero";
-    break;
+    return "towardzero";
   default:
-    break;
+    return std::nullopt;
   }
-  return RoundingStr;
 }
 
 std::optional<fp::ExceptionBehavior>
@@ -139,16 +132,14 @@ convertExceptionBehaviorToBundle(fp::ExceptionBehavior UseExcept) {
   std::optional<StringRef> ExceptStr;
   switch (UseExcept) {
   case fp::ebStrict:
-    ExceptStr = "strict";
-    break;
+    return "strict";
   case fp::ebIgnore:
-    ExceptStr = "ignore";
-    break;
+    return "ignore";
   case fp::ebMayTrap:
-    ExceptStr = "maytrap";
-    break;
+    return "maytrap";
+  default:
+    return std::nullopt;
   }
-  return ExceptStr;
 }
 
 Intrinsic::ID getConstrainedIntrinsicID(const Instruction &Instr) {
diff --git a/llvm/lib/IR/IRBuilder.cpp b/llvm/lib/IR/IRBuilder.cpp
@@ -126,7 +126,7 @@ CallInst *IRBuilderBase::CreateCall(FunctionType *FTy, Value *Callee,
   if (const auto *Func = dyn_cast<Function>(Callee))
     if (Intrinsic::ID ID = Func->getIntrinsicID())
       if (IntrinsicInst::isFloatingPointOperation(ID)) {
-        // If the builder has non-default floating-point options, add
+        // If the builder specifies non-default floating-point options, add
         // corresponding operand bundle unless a bundle with such tag is already
         // present.
         bool NeedRounding;
@@ -141,23 +141,24 @@ CallInst *IRBuilderBase::CreateCall(FunctionType *FTy, Value *Callee,
           assert(DefaultConstrainedExcept == fp::ebIgnore &&
                  "FP exception in default mode must be ignored");
         }
+        // Options specified by bundles have higher precedence.
+        for (const auto &Bundle : OpBundles) {
+          if (NeedRounding && Bundle.getTag() == "fp.round")
+            NeedRounding = false;
+          if (NeedExceptions && Bundle.getTag() == "fp.except")
+            NeedExceptions = false;
+        }
         if (NeedRounding || NeedExceptions) {
-          for (const auto &Bundle : OpBundles) {
-            if (NeedRounding && Bundle.getTag() == "fp.round")
-              NeedRounding = false;
-            if (NeedExceptions && Bundle.getTag() == "fp.except")
-              NeedExceptions = false;
-          }
-          if (NeedRounding || NeedExceptions) {
-            ActualBundles.append(OpBundles.begin(), OpBundles.end());
-            if (NeedRounding)
-              createRoundingBundle(ActualBundles, DefaultConstrainedRounding);
-            if (NeedExceptions)
-              createExceptionBundle(ActualBundles, DefaultConstrainedExcept);
-            ActualBundlesRef = ActualBundles;
-          }
+          ActualBundles.append(OpBundles.begin(), OpBundles.end());
+          if (NeedRounding)
+            createRoundingBundle(ActualBundles, DefaultConstrainedRounding);
+          if (NeedExceptions)
+            createExceptionBundle(ActualBundles, DefaultConstrainedExcept);
+          ActualBundlesRef = ActualBundles;
         }
         if (IsFPConstrained) {
+          // Due to potential reading FP exception bits, in strictfp mode the
+          // memory effects must include read/write access to FPE.
           MemoryEffects FME = Func->getMemoryEffects();
           NeedUpdateMemoryEffects = !FME.doesAccessInaccessibleMem();
         }
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp
@@ -649,7 +649,7 @@ void AMDGPUAtomicOptimizerImpl::optimizeAtomic(Instruction &I,
   IRBuilder<> B(&I);
 
   if (AtomicRMWInst::isFPOperation(Op)) {
-    B.setIsFPConstrained(I.getFunction()->hasFnAttribute(Attribute::StrictFP));
+    B.resetModeToStrictFP(I.getFunction()->hasFnAttribute(Attribute::StrictFP));
   }
 
   // If we are in a pixel shader, because of how we have to mask out helper
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp b/llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
@@ -654,7 +654,7 @@ bool AMDGPULibCalls::fold(CallInst *CI) {
 
   IRBuilder<> B(CI);
   if (CI->isStrictFP())
-    B.setIsFPConstrained(true);
+    B.resetModeToStrictFP(true);
 
   if (FPMathOperator *FPOp = dyn_cast<FPMathOperator>(CI)) {
     // Under unsafe-math, evaluate calls if possible.
diff --git a/llvm/lib/Transforms/Utils/LibCallsShrinkWrap.cpp b/llvm/lib/Transforms/Utils/LibCallsShrinkWrap.cpp
@@ -103,7 +103,7 @@ class LibCallsShrinkWrap : public InstVisitor<LibCallsShrinkWrap> {
     if (!Arg->getType()->isFloatTy())
       V = ConstantFoldCastInstruction(Instruction::FPExt, V, Arg->getType());
     if (BBBuilder.GetInsertBlock()->getParent()->hasFnAttribute(Attribute::StrictFP))
-      BBBuilder.setIsFPConstrained(true);
+      BBBuilder.resetModeToStrictFP(true);
     return BBBuilder.CreateFCmp(Cmp, Arg, V);
   }
 
diff --git a/llvm/lib/Transforms/Utils/LowerAtomic.cpp b/llvm/lib/Transforms/Utils/LowerAtomic.cpp
@@ -124,7 +124,7 @@ Value *llvm::buildAtomicRMWValue(AtomicRMWInst::BinOp Op,
 
 bool llvm::lowerAtomicRMWInst(AtomicRMWInst *RMWI) {
   IRBuilder<> Builder(RMWI);
-  Builder.setIsFPConstrained(
+  Builder.resetModeToStrictFP(
       RMWI->getFunction()->hasFnAttribute(Attribute::StrictFP));
 
   Value *Ptr = RMWI->getPointerOperand();
diff --git a/llvm/test/Other/print-fp-memeffects.ll b/llvm/test/Other/print-fp-memeffects.ll
@@ -0,0 +1,15 @@
+; RUN: opt %s -print-fp-memory-effects -S | FileCheck %s --match-full-lines
+
+define float @test_00(float %x) {
+  %res = call float @llvm.nearbyint.f32(float %x) [ "fpe.round"(metadata !"upward"), "fpe.except"(metadata !"ignore") ]
+  ret float %res
+; CHECK-LABEL: define float @test_00({{.*}}
+; CHECK:       {{.*}} = call float @llvm.nearbyint.f32({{.*}}]
+}
+
+define float @test_01(float %x) strictfp {
+  %res = call float @llvm.nearbyint.f32(float %x) [ "fpe.round"(metadata !"upward"), "fpe.except"(metadata !"strict") ]
+  ret float %res
+; CHECK-LABEL: define float @test_01({{.*}}
+; CHECK:       {{.*}} = call float @llvm.nearbyint.f32({{.*}}] ; fpe=[rw]
+}
diff --git a/llvm/unittests/IR/IRBuilderTest.cpp b/llvm/unittests/IR/IRBuilderTest.cpp

Original file line number	Diff line number	Diff line change
`@@ -1089,7 +1089,7 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,`
`1089`	`1089`	`if ((FD && (FD->UsesFPIntrin() \|\| FD->hasAttr<StrictFPAttr>())) \|\|`
`1090`	`1090`	`(!FD && (FPExceptionBehavior != llvm::fp::ebIgnore \|\|`
`1091`	`1091`	`RM != llvm::RoundingMode::NearestTiesToEven))) {`
`1092`		`- Builder.setIsFPConstrained(true, false);`
	`1092`	`+ Builder.setIsFPConstrained(true);`
`1093`	`1093`	`Fn->addFnAttr(llvm::Attribute::StrictFP);`
`1094`	`1094`	`}`
`1095`	`1095`