[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