-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[TTI] Introduce getInstructionUniformity API for flexible uniformity analysis #137639
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
base: main
Are you sure you want to change the base?
[TTI] Introduce getInstructionUniformity API for flexible uniformity analysis #137639
Conversation
|
@llvm/pr-subscribers-backend-nvptx @llvm/pr-subscribers-backend-amdgpu Author: Pankaj Dwivedi (PankajDwivedi-25) ChangesThis patch introduces a new target hook Currently, UniformityAnalysis categorizes instructions into a fixed set of InstructionUniformity values (Default, AlwaysUniform, NeverUniform). This hook allows targets to override and implement custom uniformity-propagation rules for such cases. Full diff: https://github.com/llvm/llvm-project/pull/137639.diff 7 Files Affected:
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfo.h b/llvm/include/llvm/Analysis/TargetTransformInfo.h
index 022530dc846ea..9af5006ce9c6d 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfo.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfo.h
@@ -23,6 +23,7 @@
#include "llvm/ADT/APInt.h"
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Uniformity.h"
#include "llvm/Analysis/IVDescriptors.h"
#include "llvm/IR/FMF.h"
#include "llvm/IR/InstrTypes.h"
@@ -1916,6 +1917,13 @@ class TargetTransformInfo {
const Function &F,
SmallVectorImpl<std::pair<StringRef, int64_t>> &LB) const;
+ /// Target can implement more complex patterns for getting Uniformity of an
+ /// instruction.Currently Uniformity analysis catagorises instructions with a
+ /// fixed set of InstructionUniformity values: Default, AlwaysUniform and
+ /// NeverUniform.
+ std::optional<InstructionUniformity>
+ getInstructionUniformity(const Instruction &I) const;
+
private:
std::unique_ptr<const TargetTransformInfoImplBase> TTIImpl;
};
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index 990252b1e5743..5bee462575181 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -1147,6 +1147,11 @@ class TargetTransformInfoImplBase {
const Function &F,
SmallVectorImpl<std::pair<StringRef, int64_t>> &LB) const {}
+ virtual std::optional<InstructionUniformity>
+ getInstructionUniformity(const Instruction &I) const {
+ return std::nullopt;
+ }
+
protected:
// Obtain the minimum required size to hold the value (without the sign)
// In case of a vector it returns the min required size for one element.
diff --git a/llvm/lib/Analysis/TargetTransformInfo.cpp b/llvm/lib/Analysis/TargetTransformInfo.cpp
index 8548afea72964..50157a7714bf7 100644
--- a/llvm/lib/Analysis/TargetTransformInfo.cpp
+++ b/llvm/lib/Analysis/TargetTransformInfo.cpp
@@ -1476,6 +1476,11 @@ void TargetTransformInfo::collectKernelLaunchBounds(
return TTIImpl->collectKernelLaunchBounds(F, LB);
}
+std::optional<InstructionUniformity>
+TargetTransformInfo::getInstructionUniformity(const Instruction &I) const {
+ return TTIImpl->getInstructionUniformity(I);
+}
+
TargetTransformInfoImplBase::~TargetTransformInfoImplBase() = default;
TargetIRAnalysis::TargetIRAnalysis() : TTICallback(&getDefaultTTI) {}
diff --git a/llvm/lib/Analysis/UniformityAnalysis.cpp b/llvm/lib/Analysis/UniformityAnalysis.cpp
index 2101fdfacfc8f..2fc6f523139a7 100644
--- a/llvm/lib/Analysis/UniformityAnalysis.cpp
+++ b/llvm/lib/Analysis/UniformityAnalysis.cpp
@@ -35,7 +35,20 @@ template <> void llvm::GenericUniformityAnalysisImpl<SSAContext>::initialize() {
markDivergent(I);
else if (TTI->isAlwaysUniform(&I))
addUniformOverride(I);
+ else if (auto Uniformity = TTI->getInstructionUniformity(I)) {
+ switch (*Uniformity) {
+ case InstructionUniformity::AlwaysUniform:
+ addUniformOverride(I);
+ break;
+ case InstructionUniformity::NeverUniform:
+ markDivergent(I);
+ break;
+ case InstructionUniformity::Default:
+ break;
+ }
+ }
}
+
for (auto &Arg : F.args()) {
if (TTI->isSourceOfDivergence(&Arg)) {
markDivergent(&Arg);
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
index 204d3df546bbf..5c59847dfeb62 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
@@ -1422,3 +1422,24 @@ void GCNTTIImpl::collectKernelLaunchBounds(
LB.push_back({"amdgpu-waves-per-eu[0]", WavesPerEU.first});
LB.push_back({"amdgpu-waves-per-eu[1]", WavesPerEU.second});
}
+
+std::optional<InstructionUniformity>
+GCNTTIImpl::getInstructionUniformity(const Instruction &I) const {
+ if (const auto *II = dyn_cast<IntrinsicInst>(&I)) {
+ // We can define the custom rules for the intrinsics uniformity, depending
+ // on argument.
+ switch (II->getIntrinsicID()) {
+ case Intrinsic::amdgcn_permlane64:
+ // If either operand is uniform, the result is uniform.
+ for (unsigned Arg_i = 0, NumArg = II->arg_size(); Arg_i < NumArg;
+ Arg_i++) {
+ if (!isSourceOfDivergence(II->getArgOperand(Arg_i)))
+ return InstructionUniformity::AlwaysUniform;
+ }
+ return InstructionUniformity::Default;
+ default:
+ break;
+ }
+ }
+ return std::nullopt;
+}
\ No newline at end of file
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.h b/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.h
index f6f7bd4bfcf5b..bea0b024d745b 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.h
@@ -290,6 +290,8 @@ class GCNTTIImpl final : public BasicTTIImplBase<GCNTTIImpl> {
void collectKernelLaunchBounds(
const Function &F,
SmallVectorImpl<std::pair<StringRef, int64_t>> &LB) const override;
+ std::optional<InstructionUniformity>
+ getInstructionUniformity(const Instruction &I) const override;
};
} // end namespace llvm
diff --git a/llvm/test/Analysis/UniformityAnalysis/AMDGPU/uniform_intrinsic.ll b/llvm/test/Analysis/UniformityAnalysis/AMDGPU/uniform_intrinsic.ll
new file mode 100644
index 0000000000000..4bb89516b2e81
--- /dev/null
+++ b/llvm/test/Analysis/UniformityAnalysis/AMDGPU/uniform_intrinsic.ll
@@ -0,0 +1,25 @@
+; RUN: opt -mtriple amdgcn-unknown-amdhsa -passes='print<uniformity>' -disable-output %s 2>&1 | FileCheck %s
+
+; CHECK: ALL VALUES UNIFORM
+define amdgpu_kernel void @permlane64_constant(ptr addrspace(1) %out) {
+ %v = call i32 @llvm.amdgcn.permlane64(i32 7)
+ store i32 %v, ptr addrspace(1) %out
+ ret void
+}
+
+; CHECK: ALL VALUES UNIFORM
+define amdgpu_kernel void @permlane64_uniform(ptr addrspace(1) %out, i32 %src) {
+ %v = call i32 @llvm.amdgcn.permlane64(i32 %src)
+ store i32 %v, ptr addrspace(1) %out
+ ret void
+}
+
+; CHECK: DIVERGENT: %tid = call i32 @llvm.amdgcn.workitem.id.x()
+; CHECK: DIVERGENT: %v = call i32 @llvm.amdgcn.permlane64.i32(i32 %tid)
+define amdgpu_kernel void @permlane64_nonuniform(i32 addrspace(1)* %out) {
+ %tid = call i32 @llvm.amdgcn.workitem.id.x()
+ %v = call i32 @llvm.amdgcn.permlane64(i32 %tid)
+ %out_ptr = getelementptr i32, i32 addrspace(1)* %out, i32 %tid
+ store i32 %v, i32 addrspace(1)* %out_ptr
+ ret void
+}
|
arsenm
left a comment
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 don't know what this new hook gives that the existing ones do not handle already
I think this is supposed to address #131779 (but in its current form it does not). |
|
Right, this is initial patch. Right now it doesn't do anything more than already there. I was looking for two things here.
|
8cca1b2 to
e4cc773
Compare
|
I can see the target intrinsic of this hook is currently classified as For example, I have removed one such intrinsic from the list in the .td, and now the hook can classify it as uniform if one or more of its operands are uniform. |
e4cc773 to
860b485
Compare
The way you have implemented this, For example, llvm.amdgcn.permlane16 has extra I really think we need a more flexible target hook that can implement any arbitrary logic for the uniformity of the result of an operation, based on the uniformity of each of its operands. |
arsenm
left a comment
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.
Don't we already have special context dependent uniformity with the existing API? e.g. identifying math based on the work item ID
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 | ||
| ; RUN: opt -mtriple amdgcn-- -passes='print<uniformity>' -disable-output %s 2>&1 | FileCheck %s | ||
|
|
||
| ; CHECK: ALL VALUES UNIFORM |
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.
These aren't always always uniform, so need test coverage for all the conditions that would make this divergent
I don't get this. you mean pattern optimizations based on work item ID? I don't see any such API existing at the IR level. |
The only way I see to achieve this is by passing the Uniformity of the operands to Targets and letting them define the custom rule. My concern is that it would be a bit expensive, also the decision of uniformity will be taken outside of UA. Let me know if you guys have any thoughts on how best this can be implemented? |
Yes we have some special known-uniform cases in GCNTTIImpl::isAlwaysUniform, but that code does not have access to the uniformity of the operands, so it cannot implement rules like "result is uniform if any operand is uniform". |
Exactly, you would need some kind of target hook where the uniformity of the operands is passed in, or where the target hook can call back into UA to query the uniformity of operands.
I don't see why it would be too expensive. If it is then we should just give up on trying to implement this feature. |
We believe it will be expensive because there's a virtual function call happening every time the UA (re)visits an intrinsic call. The approach we are exploring now is more static ... the target hook can return an enum encoding the "uniformity policy" of the intrinsic, which will be cached. Then the UA can interpret the policy every time it visits an intrinsic. That way, all the queries and decisions taken for uniformity stay within the UA implementation, and the virtual function is called only once when initializing the UA. |
Well, OK, but I think it needs to be at least flexible enough to handle cases like ""result is uniform if either of the first two operands are uniform", for intrinsics like llvm.amdgcn.permlane16. |
caa7354 to
afec697
Compare
| case InstructionUniformity::EitherOfFirstTwoOp: | ||
| return !isDivergentUse(I.getOperand(0)) || !isDivergentUse(I.getOperand(1)); |
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.
This API will work poorly for machine instrs. This will typically be the def
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.
Yeah, right, I think Op_1 and Op_2 should be checked here.
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.
If you're adding this new hook, you should aim to remove the existing hooks isSourceOfDivergence and isAlwaysUniform, since the new one is strictly more capable. This could be done as a separate NFC refactoring.
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 would strongly prefer removing the existing hooks as part of this patch. It just might expose rough corners that we need to consider in this design itself.
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.
Got it, you guys are saying instead of using those two, I should use the newly added one getInstructionUniformity.
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 would strongly prefer removing the existing hooks as part of this patch.
How about doing it as an NFC refactoring before this patch?
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.
Removing those API's will require their uses to be replaced with the new API, so the best approach would be either to do it in the current patch or after this.
|
Thank you for working on this. Sorry I just notice this. Let me share my thinking on this. Currently uniform analysis works like: identifying divergent sources (through It would be easy to switch Does this make sense? |
Thanks for your review. As per our design discussion, we want the uniformity decision to be made within the analysis pass itself rather than querying the target about it. For that purpose, during the initialization phase, we query the target and record the target-dependent uniformity for later divergence propagation. This approach will be flexible to handle any complex target-dependent uniformity, both for IR and MIRs. |
Yes, it can be seen as this way as well, currently we try to prove it Uniform. The new approach you are suggesting will also look similar. At all the places will be trying to prove divergent instead of uniform in the current patch. |
|
Sorry I just noticed the target hook I suggested can only solve part of the problem... Let's skip it. Back to the permlane16. The intrinsic is defined as |
It’s the other way around: if any of the first two operands (old, src0) is uniform, the result is uniform. The result is divergent only if both operands are divergent. |
You have chosen a very complicated example! permlane16 is like shuffle where However, |
Since the permlane16-like intrinsics only shuffle within/across 16-lane-wide row. Even the
Yes, agree on the complexity of the uniformity of the intrinsic. Maybe this is less important in real case. |
Yes, you are right of course. |
Co-authored-by: Matt Arsenault <[email protected]>
dd8c0e1 to
5647603
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
|
nhaehnle
left a comment
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.
Unrelated to the generic uniformity analysis change, there seems to be a misunderstanding by everybody in this thread about how permlane16 works. Please read up the instruction definition, y'all! Trying to rely on the uniformity of src1 and src2 is confused.
As for the uniformity analysis change itself, there are a number of ways this could go, but I think either way what we'd want is the ability to define more complex rules in the TTI itself, not via some ad-hoc enum. This could take the form of a callback like:
virtual bool isUniform(const Instruction &I, const SmallBitVector &UniformArgs);
... where UniformArgs has a bit per instruction operand indicating whether the method may assume it to be uniform or not.
Since this is an uncommon case, we may want to guard using this method by a forth InstructionUniformity enum called something like InstructionUniformity::Complex:
// If all operands are uniform, the result values are uniform. Otherwise, the result
// values may be divergent, and a custom check may be used to determine uniformity.
Custom,
llvm/include/llvm/ADT/Uniformity.h
Outdated
| /// Result value can be uniform if any of the first two use operand are | ||
| /// uniform. | ||
| AnyOfFirstTwoUseOp |
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.
That enum value seems like a really bad precedent. It's so arbitrary.
Thanks @nhaehnle for your feedback, I have addressed the changes. please let me know if there are other changes required. |
jayfoad
left a comment
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.
What happened to the idea of a preliminary NFC patch to introduce TTI::getInstructionUniformity as a replacement for isAlwaysUniform/isSourceOfDivergence?
| // Operand 2: src1 (lane select within 16-lane group) | ||
| // Operand 3: src2 (which 16-lane group) | ||
| // Result is uniform if either src0 (op 1) or src1 (op 2) is uniform |
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.
As Nicolai pointed out this is completely wrong and permlane16 is the wrong example to use to demonstrate the new functionality.
As per our previous discussion, it's better to make that part of this patch only to catch any edge case? |
I don't understand that argument. I'd much prefer to see it as a separate patch. |
Let me do it as a separate patch only. I would prefer to keeping it as a wrapper on top of isAlwaysUniform and isSourceOfDivergence. |
See: #131779
This patch introduces a new
getInstructionUniformity()API to TargetTransformInfothat provides more flexibility for targets to classify instruction uniformity. The
new API uses an InstructionUniformity enum that can express:
The old
isSourceOfDivergence()andisAlwaysUniform()APIs are kept unchanged forbackward compatibility. The new API delegates to these old APIs, providing an
incremental migration path. UniformityAnalysis is updated to exclusively use the
new API.
This change enables more precise divergence analysis for AMDGPU's permlane16 and
permlanex16 intrinsics, which can produce uniform results when their operands are
uniform, despite being previously marked as unconditionally divergent. These
intrinsics are removed from the divergence table and now return
AnyOfFirstTwoUseOp,allowing the uniformity analysis to track their actual uniformity based on operands.
For NVPTX, the new API simply wraps the existing
isSourceOfDivergencebehavior.All existing lit tests pass with this change.