[NFC][HLSL] Allow target intrinsic switching to optionally be set.

[farzonl]

This will make it easier for us to define an intrinsic optionally per backend.

[llvmbot]

@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Farzon Lotfi (farzonl)

Changes

This will make it easier for us to define an intrinsic optionally per backend.

---

Full diff: https://github.com/llvm/llvm-project/pull/117648.diff

1 Files Affected:

• (modified) clang/lib/CodeGen/CGHLSLRuntime.h (+51-35)

diff --git a/clang/lib/CodeGen/CGHLSLRuntime.h b/clang/lib/CodeGen/CGHLSLRuntime.h
index a8e0ed42b79a35..7e0a0d286b0b56 100644
--- a/clang/lib/CodeGen/CGHLSLRuntime.h
+++ b/clang/lib/CodeGen/CGHLSLRuntime.h
@@ -30,22 +30,36 @@
 #include <optional>
 #include <vector>
 
+#define GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(FunctionName,                 \
+                                                 IntrinsicPostfix)             \
+  GENERATE_HLSL_INTRINSIC_FUNCTION(FunctionName, IntrinsicPostfix, 1, 1)
+
 // A function generator macro for picking the right intrinsic
 // for the target backend
-#define GENERATE_HLSL_INTRINSIC_FUNCTION(FunctionName, IntrinsicPostfix)       \
+#define GENERATE_HLSL_INTRINSIC_FUNCTION(FunctionName, IntrinsicPostfix,       \
+                                         IncludeDXIL, IncludeSPIRV)            \
   llvm::Intrinsic::ID get##FunctionName##Intrinsic() {                         \
     llvm::Triple::ArchType Arch = getArch();                                   \
     switch (Arch) {                                                            \
-    case llvm::Triple::dxil:                                                   \
-      return llvm::Intrinsic::dx_##IntrinsicPostfix;                           \
-    case llvm::Triple::spirv:                                                  \
-      return llvm::Intrinsic::spv_##IntrinsicPostfix;                          \
+      /* Include DXIL case only if IncludeDXIL is true */                      \
+      IF_INCLUDE(IncludeDXIL, case llvm::Triple::dxil                          \
+                 : return llvm::Intrinsic::dx_##IntrinsicPostfix;)             \
+      /* Include SPIRV case only if IncludeSPIRV is true */                    \
+      IF_INCLUDE(IncludeSPIRV, case llvm::Triple::spirv                        \
+                 : return llvm::Intrinsic::spv_##IntrinsicPostfix;)            \
+                                                                               \
     default:                                                                   \
       llvm_unreachable("Intrinsic " #IntrinsicPostfix                          \
                        " not supported by target architecture");               \
     }                                                                          \
   }
 
+#define IF_INCLUDE(Condition, Code) IF_INCLUDE_IMPL(Condition, Code)
+#define IF_INCLUDE_IMPL(Condition, Code) IF_INCLUDE_##Condition(Code)
+
+#define IF_INCLUDE_1(Code) Code
+#define IF_INCLUDE_0(Code)
+
 namespace llvm {
 class GlobalVariable;
 class Function;
@@ -72,36 +86,38 @@ class CGHLSLRuntime {
   // Start of reserved area for HLSL intrinsic getters.
   //===----------------------------------------------------------------------===//
 
-  GENERATE_HLSL_INTRINSIC_FUNCTION(All, all)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Any, any)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Cross, cross)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Degrees, degrees)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Frac, frac)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Length, length)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Lerp, lerp)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Normalize, normalize)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Rsqrt, rsqrt)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Saturate, saturate)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Sign, sign)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Step, step)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Radians, radians)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(ThreadId, thread_id)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(FDot, fdot)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(SDot, sdot)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(UDot, udot)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Dot4AddI8Packed, dot4add_i8packed)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(Dot4AddU8Packed, dot4add_u8packed)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(WaveActiveAnyTrue, wave_any)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(WaveActiveCountBits, wave_active_countbits)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(WaveIsFirstLane, wave_is_first_lane)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(WaveReadLaneAt, wave_readlane)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(FirstBitUHigh, firstbituhigh)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(FirstBitSHigh, firstbitshigh)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(NClamp, nclamp)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(SClamp, sclamp)
-  GENERATE_HLSL_INTRINSIC_FUNCTION(UClamp, uclamp)
-
-  GENERATE_HLSL_INTRINSIC_FUNCTION(CreateHandleFromBinding, handle_fromBinding)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(All, all)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Any, any)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Cross, cross)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Degrees, degrees)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Frac, frac)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Length, length)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Lerp, lerp)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Normalize, normalize)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Rsqrt, rsqrt)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Saturate, saturate)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Sign, sign)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Step, step)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Radians, radians)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(ThreadId, thread_id)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(FDot, fdot)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(SDot, sdot)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(UDot, udot)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Dot4AddI8Packed, dot4add_i8packed)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(Dot4AddU8Packed, dot4add_u8packed)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(WaveActiveAnyTrue, wave_any)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(WaveActiveCountBits,
+                                           wave_active_countbits)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(WaveIsFirstLane, wave_is_first_lane)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(WaveReadLaneAt, wave_readlane)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(FirstBitUHigh, firstbituhigh)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(FirstBitSHigh, firstbitshigh)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(NClamp, nclamp)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(SClamp, sclamp)
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(UClamp, uclamp)
+
+  GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(CreateHandleFromBinding,
+                                           handle_fromBinding)
 
   //===----------------------------------------------------------------------===//
   // End of reserved area for HLSL intrinsic getters.

[tex3d]

LGTM.

[llvm-beanz]

I feel like this is a bit of extra preprocessor complexity. Instead of having GENERATE_HLSL_INTRINSIC_FUNCTION change to take arguments, we could just define target-specific macros (i.e. GENERATE_HLSL_DX_INTRINSIC_FUNCTION and GENERATE_HLSL_SPV_INTRINSIC_FUNCTION).

I also generally think putting DEFAULT in the name of something is a bit of a red flag since you're making the name longer, but not adding relevant context.

[farzonl]

I don't see how creating target specific macros is better than just using llvm::Intrinsic::spv_* or llvm::Intrinsic::dx_*. The idea behind making it arguments is that if we have more targets after spv and dx we aren't adding an N number of new macros its just one more argument and one more IF_INCLUDE case block.

instead of DEFAULT I could rename to ALL?

• I could also make GENERATE_HLSL_DX_INTRINSIC_FUNCTION --> GENERATE_HLSL_INTRINSIC_FUNCTION(FunctionName, IntrinsicPostfix, 1, 0)

• and GENERATE_HLSL_SPV_INTRINSIC_FUNCTION --> GENERATE_HLSL_INTRINSIC_FUNCTION(FunctionName, IntrinsicPostfix, 0, 1)

[llvm-beanz]

I believe the C preprocessor is the wrong tool to do what you're trying to do here. I think this makes the code significantly harder to read, understand and debug.

There are other approaches we could take like constexpr template abstractions which would be easier to read, write and maintain than nested C preprocessor macros with dependent expansions.

[farzonl]

Fair enough closing this PR.