[clang] [llvm] Reland "[HLSL] Implement the reflect HLSL function" (PR #125599)

Farzon Lotfi via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 19 14:08:18 PST 2025


================
@@ -905,6 +903,14 @@ bool SPIRVInstructionSelector::selectExtInst(Register ResVReg,
                                              const SPIRVType *ResType,
                                              MachineInstr &I,
                                              GL::GLSLExtInst GLInst) const {
+  if (!STI.canUseExtInstSet(
+          SPIRV::InstructionSet::InstructionSet::GLSL_std_450)) {
+    std::string DiagMsg;
+    raw_string_ostream OS(DiagMsg);
+    I.print(OS, true, false, false, false);
+    DiagMsg += " is only supported with the GLSL extended instruction set.\n";
+    report_fatal_error(DiagMsg.c_str(), false);
+  }
----------------
farzonl wrote:

I'm not sure I agree. The other intrinsic selections in  `selectIntrinsic` are doing `report_fatal_error` The only difference is I don't see the report fatal error code pathes are being tested. Switchinng to `LLVMContext::diagnose` would be a new pattern and if it requires a refactor like you claim we probably want to involve the SPIRV backend stake holders depending on how large of a refactor you imagine.

Second this is a case that should never be possible. The code that emits the reflect intrinsic is only emited when the target is `spirv` via the spirv target builtins. However the opencl test is  using the `spirv32` and `spirv64` targets. This intrinsic should be invalid if the target isn't `spirv` and to me it makes sense to fail immediately.

Further their is no mixed mode where you can do ExtInst in opencl and opengl so checking `canUseExtInstSet` seems like the right way forward.

Third I agree that the error handling in `GLSLExtInst` should have its reciprocal error in the `OpenCLExtInst`. However thats beyond the scope of this PR. I wouldn't want to add error handling like that without a test and I think a test like that is out of scope for a change about the `reflect` api




https://github.com/llvm/llvm-project/pull/125599


More information about the llvm-commits mailing list