================
@@ -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);
+ }
----------------
bogner 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. Switching 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.
I am indeed asserting that many of the other uses of report_fatal_error in this
file are also wrong. However, you're probably right that it makes sense to the
existing pattern for now and clean this all up in a cohesive change.
> 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.
If the case is impossible why isn't this just an assert? In any case "failing
immediately" and "causing the program to crash" are different things, and
`report_fatal_error` is the latter.
> 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
IMO this is an argument for making a change that adds error handling to both
functions *before* this PR, so that we don't end up in a state where the
codebase is inconsistent.
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
