================
@@ -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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
