Skip to content

[DirectX] Handle named structs in DXILOpLowering in a generic way #113192

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

Closed
bogner opened this issue Oct 21, 2024 · 1 comment · Fixed by #128247
Closed

[DirectX] Handle named structs in DXILOpLowering in a generic way #113192

bogner opened this issue Oct 21, 2024 · 1 comment · Fixed by #128247
Assignees

Comments

@bogner
Copy link
Contributor

bogner commented Oct 21, 2024

In #109331 we need to replace an anonymous struct with a named %dx.types.splitdouble struct during lowering to DXIL ops, and there are a few other operations that need to undergo a similar transformation. We should add something in the DXIL.td representation of DXIL ops to say that an anonymous struct return type must be replaced with an exactly equivalent named one in this way so that the generic handling of lowering to DXIL ops from an intrinsic can be used in these cases.

Note: This should only apply to cases where the transformation is trivial. Operations like BufferLoad and AnnotateHandle use named structs but need to do more complicated transformations than simply naming a struct to get the data in the right format, so they should be explicitly out of scope here.

@bogner bogner moved this to Planning in HLSL Support Oct 21, 2024
@davidcook-msft davidcook-msft moved this from Planning to Ready in HLSL Support Oct 29, 2024
@bogner
Copy link
Contributor Author

bogner commented Oct 29, 2024

Some of the obvious cases where this will apply from skimming hctdb.py:

%dx.types.SamplePos

  • Texture2DMSGetSamplePosition
  • RenderTargetGetSamplePosition

%dx.types.fouri32

  • WaveActiveBallot
  • WaveMatch

%dx.types.splitdouble

  • SplitDouble

%dx.types.twoi32

  • IMul
  • UMul
  • UDiv
  • CycleCounterLegacy

%dx.types.u32c

  • UAddc
  • USubb

%dx.types.Dimensions

  • GetDimensions

There are also handle types (%dx.types.Handle, %dx.types.NodeHandle, %dx.types.NodeRecordHandle) that this may apply to and things like %dx.types.ResRet and %dx.types.CBufRet which are slightly more complex.

bogner added a commit to bogner/llvm-project that referenced this issue Feb 21, 2025
This removes "replaceFunctionWithNamedStructOp" and folds its
functionality into "replaceFunctionWithOp". It turns out we were
overcomplicating things and this is trivial to handle generically.

Fixes llvm#113192
@bogner bogner self-assigned this Feb 21, 2025
bogner added a commit that referenced this issue Feb 22, 2025
This removes "replaceFunctionWithNamedStructOp" and folds its
functionality into "replaceFunctionWithOp". It turns out we were
overcomplicating things and this is trivial to handle generically.

Fixes #113192
@github-project-automation github-project-automation bot moved this from Ready to Closed in HLSL Support Feb 22, 2025
Icohedron added a commit that referenced this issue Mar 6, 2025
…L op (#127137)

Fixes #99205.

- Implements the HLSL intrinsic `AddUint64` used to perform unsigned
64-bit integer addition by using pairs of unsigned 32-bit integers
instead of native 64-bit types
- The LLVM intrinsic `uadd_with_overflow` is used in the implementation
of `AddUint64` in `CGBuiltin.cpp`
- The DXIL op `UAddc` was defined in `DXIL.td`, and a lowering of the
LLVM intrinsic `uadd_with_overflow` to the `UAddc` DXIL op was
implemented in `DXILOpLowering.cpp`

Notes:
- `__builtin_addc` was not able to be used to implement `AddUint64` in
`hlsl_intrinsics.h` because its `CarryOut` argument is a pointer, and
pointers are not supported in HLSL
- A lowering of the LLVM intrinsic `uadd_with_overflow` to SPIR-V
[already
exists](https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/SPIRV/llvm-intrinsics/uadd.with.overflow.ll)
- When lowering the LLVM intrinsic `uadd_with_overflow` to the `UAddc`
DXIL op, the anonymous struct type `{ i32, i1 }` is replaced with a
named struct type `%dx.types.i32c`. This aspect of the implementation
may be changed when issue #113192 gets addressed
- Fixes issues mentioned in the comments on the original PR #125319

---------

Co-authored-by: Finn Plummer <[email protected]>
Co-authored-by: Farzon Lotfi <[email protected]>
Co-authored-by: Chris B <[email protected]>
Co-authored-by: Justin Bogner <[email protected]>
jph-13 pushed a commit to jph-13/llvm-project that referenced this issue Mar 21, 2025
…L op (llvm#127137)

Fixes llvm#99205.

- Implements the HLSL intrinsic `AddUint64` used to perform unsigned
64-bit integer addition by using pairs of unsigned 32-bit integers
instead of native 64-bit types
- The LLVM intrinsic `uadd_with_overflow` is used in the implementation
of `AddUint64` in `CGBuiltin.cpp`
- The DXIL op `UAddc` was defined in `DXIL.td`, and a lowering of the
LLVM intrinsic `uadd_with_overflow` to the `UAddc` DXIL op was
implemented in `DXILOpLowering.cpp`

Notes:
- `__builtin_addc` was not able to be used to implement `AddUint64` in
`hlsl_intrinsics.h` because its `CarryOut` argument is a pointer, and
pointers are not supported in HLSL
- A lowering of the LLVM intrinsic `uadd_with_overflow` to SPIR-V
[already
exists](https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/SPIRV/llvm-intrinsics/uadd.with.overflow.ll)
- When lowering the LLVM intrinsic `uadd_with_overflow` to the `UAddc`
DXIL op, the anonymous struct type `{ i32, i1 }` is replaced with a
named struct type `%dx.types.i32c`. This aspect of the implementation
may be changed when issue llvm#113192 gets addressed
- Fixes issues mentioned in the comments on the original PR llvm#125319

---------

Co-authored-by: Finn Plummer <[email protected]>
Co-authored-by: Farzon Lotfi <[email protected]>
Co-authored-by: Chris B <[email protected]>
Co-authored-by: Justin Bogner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

1 participant