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

[Justin Bogner via cfe-commits]

================
@@ -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