Skip to content

[DirectX] Lower llvm.lifetime.* intrinsics to stores when DXIL version is lower than 1.6 #147432

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

Merged
merged 5 commits into from
Jul 8, 2025

Conversation

Icohedron
Copy link
Contributor

@Icohedron Icohedron commented Jul 8, 2025

Fixes #147394

References DXC for the implementation logic:
https://github.com/microsoft/DirectXShaderCompiler/blob/d751c827ed3b61e87fdf57d0f424cb2d7af30cd0/lib/HLSL/DxilPreparePasses.cpp#L693-L699

If DXIL Version < 1.6 then replace lifetime intrinsics with stores

  • For validator version >= 1.6, store an undef
  • For validator version < 1.6, store zeros

else keep the lifetime intrinsics in the DXIL.

After this PR, the number of DML shaders failing validation due to #146974 is reduced from 157 to 50.

Copy link

github-actions bot commented Jul 8, 2025

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:
git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 'HEAD~1' HEAD llvm/test/CodeGen/DirectX/legalize-lifetimes-valver-1.6.ll llvm/lib/Target/DirectX/DXILOpLowering.cpp llvm/test/CodeGen/DirectX/legalize-lifetimes-valver-1.5.ll

The following files introduce new uses of undef:

  • llvm/lib/Target/DirectX/DXILOpLowering.cpp
  • llvm/test/CodeGen/DirectX/legalize-lifetimes-valver-1.6.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

@Icohedron
Copy link
Contributor Author

Icohedron commented Jul 8, 2025

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:
The following files introduce new uses of undef:

  • llvm/lib/Target/DirectX/DXILOpLowering.cpp
  • llvm/test/CodeGen/DirectX/legalize-lifetimes-valver-1.6.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

Since the undef is just for DXIL, it should be fine to ignore this? There are other uses of UndefValue::get() within DXILOpLowering.cpp and DXIL tests containing undef.

@farzonl
Copy link
Member

farzonl commented Jul 8, 2025

Since the undef is just for DXIL, it should be fine to ignore this? There are other uses of UndefValue::get() within DXILOpLowering.cpp and DXIL tests containing undef.

This should be fine DXC will always need undef since llvm 3.7 uses it.

Copy link
Contributor

@alsepkow alsepkow left a comment

Choose a reason for hiding this comment

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

LGTM. Assuming farzons feedback is addressed.

@Icohedron Icohedron merged commit d47c126 into llvm:main Jul 8, 2025
9 of 10 checks passed
@Icohedron
Copy link
Contributor Author

PR #149310 has broken a test implemented by this PR (llvm/test/CodeGen/DirectX/legalize-lifetimes-valver-1.6.ll) due to lifetime intrinsics being used on bitcast instructions.

I will revert this PR and rethink how to implement lifetime legalization for DXIL.

Icohedron added a commit to Icohedron/llvm-project that referenced this pull request Jul 21, 2025
…XIL version is lower than 1.6 (llvm#147432)"

This reverts commit d47c126.
Icohedron added a commit that referenced this pull request Jul 21, 2025
…XIL version is lower than 1.6 (#147432)" (#149874)

This PR reverts commit d47c126
(corresponding to PR #147432) to fix a build failure caused by #149310
Icohedron added a commit that referenced this pull request Jul 21, 2025
…s when DXIL version is lower than 1.6 (#147432)"" (#149882)

Reverts #149874

Reverted the wrong PR by mistake.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DirectX] llvm.lifetime.start/.end intrinsics should not be emitted in DXIL versions < 1.6
4 participants