Address code review feedback.

maarquitos14 · maarquitos14 · commit a3532950729b · 2025-07-16T20:28:12.000+02:00
diff --git a/llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp b/llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
@@ -572,64 +572,32 @@ void SPIRVAsmPrinter::outputExecutionMode(const Module &M) {
         // When SPV_KHR_float_controls2 is enabled, ContractionOff is
         // deprecated. We need to use FPFastMathDefault with the appropriate
         // flags instead. Since FPFastMathDefault takes a target type, we need
-        // to emit it for each floating-point type to match the effect of
-        // ContractionOff. As of now, there are 4 FP types: fp16, fp32, fp64 and
-        // fp128.
-        constexpr size_t NumFPTypes = 4;
-        for (size_t i = 0; i < NumFPTypes; ++i) {
+        // to emit it for each floating-point type that exists in the module
+        // to match the effect of ContractionOff. As of now, there are 4 FP
+        // types: fp16, fp32 and fp64.
+        for (const MachineInstr *MI :
+             MAI->getMSInstrs(SPIRV::MB_TypeConstVars)) {
+          // Skip if the instruction is not OpTypeFloat.
+          if (MI->getOpcode() != SPIRV::OpTypeFloat)
+            continue;
+
+          // Skip if the target type is not fp16, fp32, fp64.
+          const unsigned OpTypeFloatSize = MI->getOperand(1).getImm();
+          if (OpTypeFloatSize != 16 && OpTypeFloatSize != 32 &&
+              OpTypeFloatSize != 64) {
+            continue;
+          }
+
           MCInst Inst;
           Inst.setOpcode(SPIRV::OpExecutionMode);
           Inst.addOperand(MCOperand::createReg(FReg));
           unsigned EM =
               static_cast<unsigned>(SPIRV::ExecutionMode::FPFastMathDefault);
           Inst.addOperand(MCOperand::createImm(EM));
-
-          Type *TargetType = nullptr;
-          switch (i) {
-          case 0:
-            TargetType = Type::getHalfTy(M.getContext());
-            break;
-          case 1:
-            TargetType = Type::getFloatTy(M.getContext());
-            break;
-          case 2:
-            TargetType = Type::getDoubleTy(M.getContext());
-            break;
-          case 3:
-            TargetType = Type::getFP128Ty(M.getContext());
-            break;
-          }
-          assert(TargetType && "Invalid target type for FPFastMathDefault");
-
-          // Find the SPIRV type matching the target type. We'll go over all the
-          // TypeConstVars instructions in the SPIRV module and find the one
-          // that matches the target type. We know the target type is a
-          // floating-point type, so we can skip anything different than
-          // OpTypeFloat. Then, we need to check the bitwidth.
-          bool SPIRVTypeFound = false;
-          for (const MachineInstr *MI :
-               MAI->getMSInstrs(SPIRV::MB_TypeConstVars)) {
-            // Skip if the instruction is not OpTypeFloat.
-            if (MI->getOpcode() != SPIRV::OpTypeFloat)
-              continue;
-
-            // Skip if TargetTy bitwidth doesn't match MI->getOperand(1), which
-            // is the SPIRV type bit width.
-            if (TargetType->getScalarSizeInBits() != MI->getOperand(1).getImm())
-              continue;
-
-            SPIRVTypeFound = true;
-            const MachineFunction *MF = MI->getMF();
-            MCRegister TypeReg =
-                MAI->getRegisterAlias(MF, MI->getOperand(0).getReg());
-            Inst.addOperand(MCOperand::createReg(TypeReg));
-          }
-
-          if (!SPIRVTypeFound) {
-            // The module does not contain this FP type, so we don't need to
-            // emit FPFastMathDefault for it.
-            continue;
-          }
+          const MachineFunction *MF = MI->getMF();
+          MCRegister TypeReg =
+              MAI->getRegisterAlias(MF, MI->getOperand(0).getReg());
+          Inst.addOperand(MCOperand::createReg(TypeReg));
           // We only end up here because there is no "spirv.ExecutionMode"
           // metadata, so that means no FPFastMathDefault. Therefore, we only
           // need to make sure AllowContract is set to 0, as the rest of flags.
@@ -694,47 +662,43 @@ void SPIRVAsmPrinter::outputAnnotations(const Module &M) {
 }
 
 void SPIRVAsmPrinter::outputFPFastMathDefaultInfo() {
+  // Collect the SPIRVTypes that are OpTypeFloat.
+  std::vector<const MachineInstr *> SPIRVFloatTypes;
+  for (const MachineInstr *MI : MAI->getMSInstrs(SPIRV::MB_TypeConstVars)) {
+    // Skip if the instruction is not OpTypeFloat.
+    if (MI->getOpcode() != SPIRV::OpTypeFloat)
+      continue;
+
+    // Collect the SPIRV type.
+    SPIRVFloatTypes.push_back(MI);
+  }
+
   for (const auto &[Func, FPFastMathDefaultInfoVec] :
        MAI->FPFastMathDefaultInfoMap) {
-    for (const auto &FPFastMathDefaultInfo : FPFastMathDefaultInfoVec) {
+    if (FPFastMathDefaultInfoVec.empty())
+      continue;
+
+    for (const MachineInstr *MI : SPIRVFloatTypes) {
+      unsigned OpTypeFloatSize = MI->getOperand(1).getImm();
+      unsigned Index = computeFPFastMathDefaultInfoVecIndex(OpTypeFloatSize);
+      assert(Index < FPFastMathDefaultInfoVec.size() &&
+             "Index out of bounds for FPFastMathDefaultInfoVec");
+      const auto &FPFastMathDefaultInfo = FPFastMathDefaultInfoVec[Index];
+      assert(FPFastMathDefaultInfo.Ty &&
+             "Expected target type for FPFastMathDefaultInfo");
+      assert(FPFastMathDefaultInfo.Ty->getScalarSizeInBits() ==
+                 OpTypeFloatSize &&
+             "Mismatched float type size");
       MCInst Inst;
       Inst.setOpcode(SPIRV::OpExecutionMode);
       MCRegister FuncReg = MAI->getFuncReg(Func);
       assert(FuncReg.isValid());
       Inst.addOperand(MCOperand::createReg(FuncReg));
       Inst.addOperand(
           MCOperand::createImm(SPIRV::ExecutionMode::FPFastMathDefault));
-
-      // Find the SPIRV type matching the target type. We'll go over all the
-      // TypeConstVars instructions in the SPIRV module and find the one that
-      // matches the target type. We know the target type is a floating-point
-      // type, so we can skip anything different than OpTypeFloat. Then, we
-      // need to check the bitwidth.
-      const Type *TargetTy = FPFastMathDefaultInfo.Ty;
-      assert(TargetTy && "Expected target type");
-      bool SPIRVTypeFound = false;
-      for (const MachineInstr *MI : MAI->getMSInstrs(SPIRV::MB_TypeConstVars)) {
-        // Skip if the instruction is not OpTypeFloat.
-        if (MI->getOpcode() != SPIRV::OpTypeFloat)
-          continue;
-
-        // Skip if TargetTy bitwidth doesn't match MI->getOperand(1), which is
-        // the SPIRV type bit width.
-        if (TargetTy->getScalarSizeInBits() != MI->getOperand(1).getImm())
-          continue;
-
-        SPIRVTypeFound = true;
-        const MachineFunction *MF = MI->getMF();
-        MCRegister TypeReg =
-            MAI->getRegisterAlias(MF, MI->getOperand(0).getReg());
-        Inst.addOperand(MCOperand::createReg(TypeReg));
-      }
-      if (!SPIRVTypeFound) {
-        // The module does not contain this FP type, so we don't need to emit
-        // FPFastMathDefault for it.
-        continue;
-      }
-
+      MCRegister TypeReg =
+          MAI->getRegisterAlias(MI->getMF(), MI->getOperand(0).getReg());
+      Inst.addOperand(MCOperand::createReg(TypeReg));
       unsigned Flags = FPFastMathDefaultInfo.FastMathFlags;
       if (FPFastMathDefaultInfo.ContractionOff &&
           (Flags & SPIRV::FPFastMathMode::AllowContract) &&
diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
@@ -2094,25 +2094,24 @@ static unsigned getFastMathFlags(const MachineInstr &I,
 
   if (CanUseKHRFloatControls2) {
     // Error out if SPIRV::FPFastMathMode::Fast is enabled.
-    if (Flags & SPIRV::FPFastMathMode::Fast)
-      report_fatal_error(
-          "FPFastMathMode::Fast flag is deprecated and it is not "
-          "valid to use anymore.");
+    assert(!(Flags & SPIRV::FPFastMathMode::Fast) &&
+           "SPIRV::FPFastMathMode::Fast is deprecated and should not be used "
+           "anymore.");
 
     // Error out if AllowTransform is enabled without AllowReassoc and
     // AllowContract.
-    if ((Flags & SPIRV::FPFastMathMode::AllowTransform) &&
-        (!(Flags & SPIRV::FPFastMathMode::AllowReassoc) ||
-         !(Flags & SPIRV::FPFastMathMode::AllowContract)))
-      report_fatal_error(
-          "FPFastMathMode::AllowTransform flag requires AllowReassoc and "
-          "AllowContract flags to be enabled as well.");
+    assert(
+        !(Flags & SPIRV::FPFastMathMode::AllowTransform) ||
+        ((Flags & SPIRV::FPFastMathMode::AllowReassoc &&
+          Flags & SPIRV::FPFastMathMode::AllowContract)) &&
+            "SPIRV::FPFastMathMode::AllowTransform requires AllowReassoc and "
+            "AllowContract flags to be enabled as well.");
   }
 
   return Flags;
 }
 
-static bool isFastMathMathModeAvailable(const SPIRVSubtarget &ST) {
+static bool isFastMathModeAvailable(const SPIRVSubtarget &ST) {
   if (ST.isKernel())
     return true;
   if (ST.getSPIRVVersion() < VersionTuple(1, 2))
@@ -2123,7 +2122,7 @@ static bool isFastMathMathModeAvailable(const SPIRVSubtarget &ST) {
 static void handleMIFlagDecoration(
     MachineInstr &I, const SPIRVSubtarget &ST, const SPIRVInstrInfo &TII,
     SPIRV::RequirementHandler &Reqs, const SPIRVGlobalRegistry *GR,
-    SmallVector<SPIRV::FPFastMathDefaultInfo, 4> &FPFastMathDefaultInfoVec) {
+    SmallVector<SPIRV::FPFastMathDefaultInfo, 3> &FPFastMathDefaultInfoVec) {
   if (I.getFlag(MachineInstr::MIFlag::NoSWrap) && TII.canUseNSW(I) &&
       getSymbolicOperandRequirements(SPIRV::OperandCategory::DecorationOperand,
                                      SPIRV::Decoration::NoSignedWrap, ST, Reqs)
@@ -2184,7 +2183,7 @@ static void handleMIFlagDecoration(
     if (FMFlags == SPIRV::FPFastMathMode::None && !Emit)
       return;
   }
-  if (isFastMathMathModeAvailable(ST)) {
+  if (isFastMathModeAvailable(ST)) {
     Register DstReg = I.getOperand(0).getReg();
     buildOpDecorate(DstReg, I, TII, SPIRV::Decoration::FPFastMathMode,
                     {FMFlags});
@@ -2250,7 +2249,7 @@ static void patchPhis(const Module &M, SPIRVGlobalRegistry *GR,
   }
 }
 
-static SmallVector<SPIRV::FPFastMathDefaultInfo, 4> &
+static SmallVector<SPIRV::FPFastMathDefaultInfo, 3> &
 getOrCreateFPFastMathDefaultInfoVec(const Module &M,
                                     SPIRV::ModuleAnalysisInfo &MAI,
                                     const Function *F) {
@@ -2259,29 +2258,27 @@ getOrCreateFPFastMathDefaultInfoVec(const Module &M,
     return it->second;
 
   // If the map does not contain the entry, create a new one. Initialize it to
-  // contain all 4 elements sorted by bit width of target type: {half, float,
-  // double, fp128}.
-  SmallVector<SPIRV::FPFastMathDefaultInfo, 4> FPFastMathDefaultInfoVec;
+  // contain all 3 elements sorted by bit width of target type: {half, float,
+  // double}.
+  SmallVector<SPIRV::FPFastMathDefaultInfo, 3> FPFastMathDefaultInfoVec;
   FPFastMathDefaultInfoVec.emplace_back(Type::getHalfTy(M.getContext()),
                                         SPIRV::FPFastMathMode::None);
   FPFastMathDefaultInfoVec.emplace_back(Type::getFloatTy(M.getContext()),
                                         SPIRV::FPFastMathMode::None);
   FPFastMathDefaultInfoVec.emplace_back(Type::getDoubleTy(M.getContext()),
                                         SPIRV::FPFastMathMode::None);
-  FPFastMathDefaultInfoVec.emplace_back(Type::getFP128Ty(M.getContext()),
-                                        SPIRV::FPFastMathMode::None);
   return MAI.FPFastMathDefaultInfoMap[F] = std::move(FPFastMathDefaultInfoVec);
 }
 
 static SPIRV::FPFastMathDefaultInfo &getFPFastMathDefaultInfo(
-    SmallVector<SPIRV::FPFastMathDefaultInfo, 4> &FPFastMathDefaultInfoVec,
+    SmallVector<SPIRV::FPFastMathDefaultInfo, 3> &FPFastMathDefaultInfoVec,
     const Type *Ty) {
   size_t BitWidth = Ty->getScalarSizeInBits();
   int Index = computeFPFastMathDefaultInfoVecIndex(BitWidth);
-  assert(Index >= 0 && Index < 4 &&
-         "Expected FPFastMathDefaultInfo for half, float, double, or fp128");
-  assert(FPFastMathDefaultInfoVec.size() == 4 &&
-         "Expected FPFastMathDefaultInfoVec to have exactly 4 elements");
+  assert(Index >= 0 && Index < 3 &&
+         "Expected FPFastMathDefaultInfo for half, float, or double");
+  assert(FPFastMathDefaultInfoVec.size() == 3 &&
+         "Expected FPFastMathDefaultInfoVec to have exactly 3 elements");
   return FPFastMathDefaultInfoVec[Index];
 }
 
@@ -2317,7 +2314,7 @@ static void collectFPFastMathDefaults(const Module &M,
             cast<ConstantInt>(
                 cast<ConstantAsMetadata>(MDN->getOperand(3))->getValue())
                 ->getZExtValue();
-        SmallVector<SPIRV::FPFastMathDefaultInfo, 4> &FPFastMathDefaultInfoVec =
+        SmallVector<SPIRV::FPFastMathDefaultInfo, 3> &FPFastMathDefaultInfoVec =
             getOrCreateFPFastMathDefaultInfoVec(M, MAI, F);
         SPIRV::FPFastMathDefaultInfo &Info =
             getFPFastMathDefaultInfo(FPFastMathDefaultInfoVec, T);
@@ -2329,7 +2326,7 @@ static void collectFPFastMathDefaults(const Module &M,
 
         // We need to save this info for every possible FP type, i.e. {half,
         // float, double, fp128}.
-        SmallVector<SPIRV::FPFastMathDefaultInfo, 4> &FPFastMathDefaultInfoVec =
+        SmallVector<SPIRV::FPFastMathDefaultInfo, 3> &FPFastMathDefaultInfoVec =
             getOrCreateFPFastMathDefaultInfoVec(M, MAI, F);
         for (SPIRV::FPFastMathDefaultInfo &Info : FPFastMathDefaultInfoVec) {
           Info.ContractionOff = true;
@@ -2342,14 +2339,13 @@ static void collectFPFastMathDefaults(const Module &M,
                 cast<ConstantAsMetadata>(MDN->getOperand(2))->getValue())
                 ->getZExtValue();
         // We need to save this info only for the FP type with TargetWidth.
-        SmallVector<SPIRV::FPFastMathDefaultInfo, 4> &FPFastMathDefaultInfoVec =
+        SmallVector<SPIRV::FPFastMathDefaultInfo, 3> &FPFastMathDefaultInfoVec =
             getOrCreateFPFastMathDefaultInfoVec(M, MAI, F);
         int Index = computeFPFastMathDefaultInfoVecIndex(TargetWidth);
-        assert(Index >= 0 && Index < 4 &&
-               "Expected FPFastMathDefaultInfo for half, float, double, or "
-               "fp128");
-        assert(FPFastMathDefaultInfoVec.size() == 4 &&
-               "Expected FPFastMathDefaultInfoVec to have exactly 4 elements");
+        assert(Index >= 0 && Index < 3 &&
+               "Expected FPFastMathDefaultInfo for half, float, or double");
+        assert(FPFastMathDefaultInfoVec.size() == 3 &&
+               "Expected FPFastMathDefaultInfoVec to have exactly 3 elements");
         FPFastMathDefaultInfoVec[Index].SignedZeroInfNanPreserve = true;
       }
     }
diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.h b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.h
@@ -189,9 +189,9 @@ struct ModuleAnalysisInfo {
   // The table maps function pointers to their default FP fast math info. It can
   // be assumed that the SmallVector is sorted by the bit width of the type. The
   // first element is the smallest bit width, and the last element is the
-  // largest bit width, therefore, we will have {half, float, double, fp128} in
+  // largest bit width, therefore, we will have {half, float, double} in
   // the order of their bit widths.
-  DenseMap<const Function *, SmallVector<FPFastMathDefaultInfo, 4>>
+  DenseMap<const Function *, SmallVector<FPFastMathDefaultInfo, 3>>
       FPFastMathDefaultInfoMap;
 
   MCRegister getFuncReg(const Function *F) {
diff --git a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
@@ -568,8 +568,6 @@ Type *parseBasicTypeName(StringRef &TypeName, LLVMContext &Ctx) {
     return Type::getFloatTy(Ctx);
   else if (TypeName.consume_front("double"))
     return Type::getDoubleTy(Ctx);
-  else if (TypeName.consume_front("fp128"))
-    return Type::getFP128Ty(Ctx);
 
   // Unable to recognize SPIRV type name
   return nullptr;
@@ -1041,10 +1039,8 @@ size_t computeFPFastMathDefaultInfoVecIndex(size_t BitWidth) {
     return 1;
   case 64: // double
     return 2;
-  case 128: // fp128
-    return 3;
   default:
-    report_fatal_error("Expected BitWidth to be 16, 32, 64, or 128", false);
+    report_fatal_error("Expected BitWidth to be 16, 32, 64", false);
   }
   assert(false && "Unreachable code");
   // This return is just to avoid compiler warnings.
diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_float_controls2/decoration.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_float_controls2/decoration.ll
@@ -50,9 +50,6 @@
 ; CHECK: OpDecorate %[[#maxRes]] FPFastMathMode NotNaN|NotInf|NSZ|AllowRecip|AllowContract|AllowReassoc|AllowTransform 
 ; CHECK: OpDecorate %[[#maxCommonRes:]] FPFastMathMode NotNaN|NotInf 
 
-target datalayout = "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
-target triple = "spir"
-
 ; Function Attrs: convergent mustprogress nofree nounwind willreturn memory(none)
 declare spir_func float @_Z4fmodff(float, float)
 declare dso_local spir_func noundef nofpclass(nan inf) float @_Z16__spirv_ocl_fmaxff(float noundef nofpclass(nan inf), float noundef nofpclass(nan inf)) local_unnamed_addr #1
diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_float_controls2/exec_mode.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_float_controls2/exec_mode.ll
@@ -24,17 +24,12 @@ entry:
   ret void
 }
 
-define dso_local dllexport spir_kernel void @k_float_controls_fp128(fp128 %fp) {
+define dso_local dllexport spir_kernel void @k_float_controls_all(half %h, bfloat %b, float %f, double %d) {
 entry:
   ret void
 }
 
-define dso_local dllexport spir_kernel void @k_float_controls_all(half %h, bfloat %b, float %f, double %d, fp128 %fp) {
-entry:
-  ret void
-}
-
-!spirv.ExecutionMode = !{!17, !18, !19, !20, !21, !22, !23, !24, !25, !26}
+!spirv.ExecutionMode = !{!17, !18, !19, !20, !22, !23, !24, !25}
 
 ; CHECK: OpEntryPoint Kernel %[[#KERNEL_HALF:]] "k_float_controls_half"
 !0 = !{ptr @k_float_controls_half, !"k_float_controls_half", !6, i32 0, !6, !7, !8, i32 0, i32 0}
@@ -48,9 +43,6 @@ entry:
 ; CHECK: OpEntryPoint Kernel %[[#KERNEL_DOUBLE:]] "k_float_controls_double"
 !3 = !{ptr @k_float_controls_double, !"k_float_controls_double", !6, i32 0, !6, !7, !8, i32 0, i32 0}
 
-; CHECK: OpEntryPoint Kernel %[[#KERNEL_FP128:]] "k_float_controls_fp128"
-!4 = !{ptr @k_float_controls_fp128, !"k_float_controls_fp128", !6, i32 0, !6, !7, !8, i32 0, i32 0}
-
 ; CHECK: OpEntryPoint Kernel %[[#KERNEL_ALL:]] "k_float_controls_all"
 !5 = !{ptr @k_float_controls_all, !"k_float_controls_all", !6, i32 0, !6, !7, !8, i32 0, i32 0}
 !6 = !{i32 2, i32 2}
@@ -69,20 +61,14 @@ entry:
 ; CHECK-DAG: OpExecutionMode %[[#KERNEL_DOUBLE]] FPFastMathDefault %[[#DOUBLE_TYPE:]] 7 
 !20 = !{ptr @k_float_controls_double, i32 6028, double poison, i32 7}
 
-; CHECK-DAG: OpExecutionMode %[[#KERNEL_FP128]] FPFastMathDefault %[[#FP128_TYPE:]] 65536
-!21 = !{ptr @k_float_controls_fp128, i32 6028, fp128 poison, i32 65536}
-
 ; CHECK-DAG: OpExecutionMode %[[#KERNEL_ALL]] FPFastMathDefault %[[#HALF_TYPE]] 131072 
 ; CHECK-DAG: OpExecutionMode %[[#KERNEL_ALL]] FPFastMathDefault %[[#FLOAT_TYPE]] 262144 
 ; CHECK-DAG: OpExecutionMode %[[#KERNEL_ALL]] FPFastMathDefault %[[#DOUBLE_TYPE]] 458752 
-; CHECK-DAG: OpExecutionMode %[[#KERNEL_ALL]] FPFastMathDefault %[[#FP128_TYPE]] 65543 
 !22 = !{ptr @k_float_controls_all, i32 6028, half poison, i32 131072}
 !23 = !{ptr @k_float_controls_all, i32 6028, bfloat poison, i32 131072}
 !24 = !{ptr @k_float_controls_all, i32 6028, float poison, i32 262144}
 !25 = !{ptr @k_float_controls_all, i32 6028, double poison, i32 458752}
-!26 = !{ptr @k_float_controls_all, i32 6028, fp128 poison, i32 65543}
 
 ; CHECK: %[[#HALF_TYPE]] = OpTypeFloat 16
 ; CHECK: %[[#FLOAT_TYPE]] = OpTypeFloat 32
 ; CHECK: %[[#DOUBLE_TYPE]] = OpTypeFloat 64
-; CHECK: %[[#FP128_TYPE]] = OpTypeFloat 128
diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_float_controls2/exec_mode2.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_float_controls2/exec_mode2.ll
