Skip to content

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

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.

Sign up for GitHub

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

Closed
farzonl wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 53 additions & 36 deletions clang/lib/CodeGen/CGHLSLRuntime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

@llvm-beanz llvm-beanz Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@farzonl farzonl Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

@llvm-beanz llvm-beanz Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@farzonl farzonl Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough closing this PR.


namespace llvm {
class GlobalVariable;
class Function;
Expand All @@ -72,37 +86,40 @@ 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(BufferUpdateCounter, bufferUpdateCounter)
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)
GENERATE_HLSL_INTRINSIC_FUNCTION_DEFAULT(BufferUpdateCounter,
bufferUpdateCounter)

//===----------------------------------------------------------------------===//
// End of reserved area for HLSL intrinsic getters.
Expand Down
Loading