-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[SPIRV] Fix SPV_KHR_expect_assume support #67793
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
This is a draft because I noticed that if we do not enable the use of the extension, extension and capability are not declared but instructions are still generated. What's the correct behaviour for this? I would think that since assume and expect do not alter the semantics we can just not issue them. The alternative is to generate an error. What do you think? Shall we just ignore the instruction if we cannot use the extension? |
If the extension is not enabled, SPIRV translator does not generate OpAssumeTrueKHR and OpExpectKHR instructions without an error message. I think we should do the same. |
@llvm/pr-subscribers-backend-spir-v ChangesSince efe0e10 changes in tests are required. Need to add extension to Extensions list Full diff: https://github.com/llvm/llvm-project/pull/67793.diff 4 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index 035989f2fe571b2..da61af7a669f149 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -12,6 +12,7 @@
//
//===----------------------------------------------------------------------===//
+#include "MCTargetDesc/SPIRVBaseInfo.h"
#include "MCTargetDesc/SPIRVMCTargetDesc.h"
#include "SPIRV.h"
#include "SPIRVGlobalRegistry.h"
@@ -1407,15 +1408,17 @@ bool SPIRVInstructionSelector::selectIntrinsic(Register ResVReg,
case Intrinsic::spv_alloca:
return selectFrameIndex(ResVReg, ResType, I);
case Intrinsic::spv_assume:
- BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpAssumeTrueKHR))
- .addUse(I.getOperand(1).getReg());
+ if (STI.canUseExtension(SPIRV::Extension::SPV_KHR_expect_assume))
+ BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpAssumeTrueKHR))
+ .addUse(I.getOperand(1).getReg());
break;
case Intrinsic::spv_expect:
- BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpExpectKHR))
- .addDef(ResVReg)
- .addUse(GR.getSPIRVTypeID(ResType))
- .addUse(I.getOperand(2).getReg())
- .addUse(I.getOperand(3).getReg());
+ if (STI.canUseExtension(SPIRV::Extension::SPV_KHR_expect_assume))
+ BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpExpectKHR))
+ .addDef(ResVReg)
+ .addUse(GR.getSPIRVTypeID(ResType))
+ .addUse(I.getOperand(2).getReg())
+ .addUse(I.getOperand(3).getReg());
break;
default:
llvm_unreachable("Intrinsic selection not implemented");
diff --git a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
index 0c185f663b63f87..cf6dfb127cdebf3 100644
--- a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
@@ -41,6 +41,10 @@ cl::list<SPIRV::Extension::Extension> Extensions(
"SPV_KHR_no_integer_wrap_decoration",
"Adds decorations to indicate that a given instruction does "
"not cause integer wrapping"),
+ clEnumValN(SPIRV::Extension::SPV_KHR_expect_assume,
+ "SPV_KHR_expect_assume",
+ "Provides additional information to a compiler, similar to "
+ "the llvm.assume and llvm.expect intrinsics."),
clEnumValN(SPIRV::Extension::SPV_KHR_bit_instructions,
"SPV_KHR_bit_instructions",
"This enables bit instructions to be used by SPIR-V modules "
diff --git a/llvm/test/CodeGen/SPIRV/assume.ll b/llvm/test/CodeGen/SPIRV/assume.ll
index 679db5d88d4fbe7..08f4cd07b01678a 100644
--- a/llvm/test/CodeGen/SPIRV/assume.ll
+++ b/llvm/test/CodeGen/SPIRV/assume.ll
@@ -1,13 +1,20 @@
-; RUN: llc -mtriple=spirv32-unknown-unknown < %s | FileCheck %s
-; RUN: llc -mtriple=spirv64-unknown-unknown < %s | FileCheck %s
+; RUN: llc -mtriple=spirv32-unknown-unknown --spirv-extensions=SPV_KHR_expect_assume < %s | FileCheck %s
+; RUN: llc -mtriple=spirv64-unknown-unknown --spirv-extensions=SPV_KHR_expect_assume < %s | FileCheck %s
+; RUN: llc -mtriple=spirv32-unknown-unknown < %s | FileCheck --check-prefix=NOEXT %s
+; RUN: llc -mtriple=spirv64-unknown-unknown < %s | FileCheck --check-prefix=NOEXT %s
; CHECK: OpCapability ExpectAssumeKHR
; CHECK-NEXT: OpExtension "SPV_KHR_expect_assume"
+; NOEXT-NOT: OpCapability ExpectAssumeKHR
+; NOEXT-NOT: OpExtension "SPV_KHR_expect_assume"
+
declare void @llvm.assume(i1)
; CHECK-DAG: %9 = OpIEqual %5 %6 %7
; CHECK-NEXT: OpAssumeTrueKHR %9
+; NOEXT: %9 = OpIEqual %5 %6 %7
+; NOEXT-NOT: OpAssumeTrueKHR %9
define void @assumeeq(i32 %x, i32 %y) {
%cmp = icmp eq i32 %x, %y
call void @llvm.assume(i1 %cmp)
diff --git a/llvm/test/CodeGen/SPIRV/expect.ll b/llvm/test/CodeGen/SPIRV/expect.ll
index 530ba7e5a49b09a..9af27965182cc04 100644
--- a/llvm/test/CodeGen/SPIRV/expect.ll
+++ b/llvm/test/CodeGen/SPIRV/expect.ll
@@ -1,5 +1,5 @@
-; RUN: llc -mtriple=spirv32-unknown-unknown < %s | FileCheck %s
-; RUN: llc -mtriple=spirv64-unknown-unknown < %s | FileCheck %s
+; RUN: llc -mtriple=spirv32-unknown-unknown --spirv-extensions=SPV_KHR_expect_assume < %s | FileCheck %s
+; RUN: llc -mtriple=spirv64-unknown-unknown --spirv-extensions=SPV_KHR_expect_assume < %s | FileCheck %s
; CHECK: OpCapability ExpectAssumeKHR
; CHECK-NEXT: OpExtension "SPV_KHR_expect_assume"
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Since efe0e10 changes in tests are required. Need to add extension to Extensions list and command line to enable use of the extension for test runs.
6f707ad
to
a152c01
Compare
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.
Otherwise, it looks good to me.
Since efe0e10 changes in tests are required. Need to add extension to Extensions list
and command line to enable use of the extension for test runs.