Skip to content

[HLSL] Add checks to the SPIRVInstructionSelector's selectExtInst functions for SPIR-V extended instruction set availability #123847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Icohedron opened this issue Jan 21, 2025 · 1 comment
Labels
backend:SPIR-V HLSL HLSL Language Support

Comments

@Icohedron
Copy link
Contributor

Certain SPIR-V instructions are supported only with the OpenCL extended instruction set but not the GLSL extended instruction set, and vice-versa.

For example, the reflect instruction is supported in GLSL, but not OpenCL. Without adding a check that the extended instruction set is usable by the current SPIR-V target, the compiler will simply crash when attempting to select G_INTRINSIC intrinsic(@llvm.spv.reflect) in an OpenCL SPIR-V target.

#122992 implements the check as follows in llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:

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);
  }
  return selectExtInst(ResVReg, ResType, I,
                       {{SPIRV::InstructionSet::GLSL_std_450, GLInst}});
}

There should be similar checks made for the other overloaded versions of the selectExtInst to keep things consistent.

  bool selectExtInst(Register ResVReg, const SPIRVType *ResType,
                     MachineInstr &I, CL::OpenCLExtInst CLInst) const;
  bool selectExtInst(Register ResVReg, const SPIRVType *ResType,
                     MachineInstr &I, CL::OpenCLExtInst CLInst,
                     GL::GLSLExtInst GLInst) const;
  bool selectExtInst(Register ResVReg, const SPIRVType *ResType,
                     MachineInstr &I, const ExtInstList &ExtInsts) const;

A non-exhaustive list of OpenCL-only instructions:

  • vstoren
  • vloadn
  • shuffle

(Note: AFAIK, these OpenCL-only instructions do not have corresponding HLSL intrinsics. I am not sure if there are any OpenCL-only instructions that have a direct HLSL intrinsic equivalent.)

A non-exhaustive list of GLSL-only instructions:

  • Reflect
  • Refract
  • FaceForward

A full list of exclusive instructions may be constructed by comparing the OpenCL extended instruction set and GLSL extended instruction set specifications.

@bogner
Copy link
Contributor

bogner commented Feb 19, 2025

As mentioned in my comment here I don't think report_fatal_error is the right kind of error handling here. We should rework these functions so that they can use LLVMContext::diagnose to bubble errors up to the driver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:SPIR-V HLSL HLSL Language Support
Projects
Status: No status
Development

No branches or pull requests

4 participants