-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[DirectX] Remove llvm.assume intrinsic #166697
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
Conversation
fixes llvm#165051 This change reverts the experiment we did for llvm#165311 While some backends seem to support llvm.assume without validation The validator itself does not so it makes more sense to just remove it.
|
@llvm/pr-subscribers-backend-directx Author: Farzon Lotfi (farzonl) Changesfixes #165051 This change reverts the experiment we did for #165311 While some backends seem to support llvm.assume without validation The validator itself does not so it makes more sense to just remove it. Full diff: https://github.com/llvm/llvm-project/pull/166697.diff 4 Files Affected:
diff --git a/llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp b/llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp
index ebb7c2607c0c8..e0d2dbde92150 100644
--- a/llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp
+++ b/llvm/lib/Target/DirectX/DXILIntrinsicExpansion.cpp
@@ -197,6 +197,7 @@ static Value *expand16BitIsNormal(CallInst *Orig) {
static bool isIntrinsicExpansion(Function &F) {
switch (F.getIntrinsicID()) {
+ case Intrinsic::assume:
case Intrinsic::abs:
case Intrinsic::atan2:
case Intrinsic::exp:
@@ -988,6 +989,9 @@ static bool expandIntrinsic(Function &F, CallInst *Orig) {
case Intrinsic::abs:
Result = expandAbs(Orig);
break;
+ case Intrinsic::assume:
+ Orig->eraseFromParent();
+ return true;
case Intrinsic::atan2:
Result = expandAtan2Intrinsic(Orig);
break;
diff --git a/llvm/lib/Target/DirectX/DXILOpLowering.cpp b/llvm/lib/Target/DirectX/DXILOpLowering.cpp
index 8720460cceb20..e46a393e50906 100644
--- a/llvm/lib/Target/DirectX/DXILOpLowering.cpp
+++ b/llvm/lib/Target/DirectX/DXILOpLowering.cpp
@@ -904,8 +904,6 @@ class OpLowerer {
case Intrinsic::dx_resource_casthandle:
// NOTE: llvm.dbg.value is supported as is in DXIL.
case Intrinsic::dbg_value:
- // NOTE: llvm.assume is supported as is in DXIL.
- case Intrinsic::assume:
case Intrinsic::not_intrinsic:
if (F.use_empty())
F.eraseFromParent();
diff --git a/llvm/test/CodeGen/DirectX/llvm_assume.ll b/llvm/test/CodeGen/DirectX/llvm_assume.ll
new file mode 100644
index 0000000000000..d739592b75d78
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/llvm_assume.ll
@@ -0,0 +1,9 @@
+; RUN: opt -S -dxil-intrinsic-expansion -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
+
+define void @test_llvm_assume(i1 %0) {
+; CHECK-LABEL: test_llvm_assume
+; CHECK-NEXT: ret void
+tail call void @llvm.assume(i1 %0)
+ret void
+}
+
diff --git a/llvm/test/tools/dxil-dis/llvm_assume.ll b/llvm/test/tools/dxil-dis/llvm_assume.ll
deleted file mode 100644
index f5be66c0d192f..0000000000000
--- a/llvm/test/tools/dxil-dis/llvm_assume.ll
+++ /dev/null
@@ -1,11 +0,0 @@
-; RUN: llc --filetype=obj %s -o - | dxil-dis -o - | FileCheck %s
-
-target triple = "dxil-pc-shadermodel6.7-library"
-
-define void @test_llvm_assume(i1 %0) {
-; CHECK-LABEL: test_llvm_assume
-; CHECK-NEXT: tail call void @llvm.assume(i1 %0)
-tail call void @llvm.assume(i1 %0)
-ret void
-}
-
|
|
|
||
| define void @test_llvm_assume(i1 %0) { | ||
| ; CHECK-LABEL: test_llvm_assume | ||
| ; CHECK-NEXT: ret void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would CHECK-NOT llvm.assume be more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking next would be better in the event someone comes along later and makes a transformation for llvm.assume. A transformation should break this test. Causing someone to come here and consider why they are doing a transformation but check-not would pass a transformation.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/32631 Here is the relevant piece of the build log for the reference |
fixes #165051
This change reverts the experiment we did for #165311
While some backends seem to support llvm.assume without validation The validator itself does not so it makes more sense to just remove it.