[NFC][Scalarizer][TargetTransformInfo] Add isVectorIntrinsicWithOverloadTypeAtArg api
#114849
Conversation
…loadTypeAtArg` This changes allows target intrinsic to specify overloaded types. This change will let us add scalarization for `asdouble`:
- change code pattern to avoid calling TTI if we don't have a target specific intrinsics - change type from unsigned to int
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-backend-directx Author: Finn Plummer (inbelic) ChangesThis changes allows target intrinsic to specify and overwrite overloaded types. This change will let us add scalarization for Full diff: https://github.com/llvm/llvm-project/pull/114849.diff 7 Files Affected:
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfo.h b/llvm/include/llvm/Analysis/TargetTransformInfo.h
index 0459941fe05cdc..26fa20be184383 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfo.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfo.h
@@ -896,6 +896,9 @@ class TargetTransformInfo {
bool isTargetIntrinsicWithScalarOpAtArg(Intrinsic::ID ID,
unsigned ScalarOpdIdx) const;
+ bool isVectorIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID,
+ int ScalarOpdIdx) const;
+
/// Estimate the overhead of scalarizing an instruction. Insert and Extract
/// are set if the demanded result elements need to be inserted and/or
/// extracted from vectors.
@@ -1969,6 +1972,8 @@ class TargetTransformInfo::Concept {
virtual bool isTargetIntrinsicTriviallyScalarizable(Intrinsic::ID ID) = 0;
virtual bool isTargetIntrinsicWithScalarOpAtArg(Intrinsic::ID ID,
unsigned ScalarOpdIdx) = 0;
+ virtual bool isVectorIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID,
+ int ScalarOpdIdx) = 0;
virtual InstructionCost getScalarizationOverhead(VectorType *Ty,
const APInt &DemandedElts,
bool Insert, bool Extract,
@@ -2530,6 +2535,11 @@ class TargetTransformInfo::Model final : public TargetTransformInfo::Concept {
return Impl.isTargetIntrinsicWithScalarOpAtArg(ID, ScalarOpdIdx);
}
+ bool isVectorIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID,
+ int ScalarOpdIdx) override {
+ return Impl.isVectorIntrinsicWithOverloadTypeAtArg(ID, ScalarOpdIdx);
+ }
+
InstructionCost getScalarizationOverhead(VectorType *Ty,
const APInt &DemandedElts,
bool Insert, bool Extract,
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index dbdfb4d8cdfa32..dd76e0b8dc8a21 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -392,6 +392,11 @@ class TargetTransformInfoImplBase {
return false;
}
+ bool isVectorIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID,
+ int ScalarOpdIdx) const {
+ return ScalarOpdIdx == -1;
+ }
+
InstructionCost getScalarizationOverhead(VectorType *Ty,
const APInt &DemandedElts,
bool Insert, bool Extract,
diff --git a/llvm/include/llvm/CodeGen/BasicTTIImpl.h b/llvm/include/llvm/CodeGen/BasicTTIImpl.h
index db3b5cddd7c1c3..3ec63c095f7c9a 100644
--- a/llvm/include/llvm/CodeGen/BasicTTIImpl.h
+++ b/llvm/include/llvm/CodeGen/BasicTTIImpl.h
@@ -798,6 +798,11 @@ class BasicTTIImplBase : public TargetTransformInfoImplCRTPBase<T> {
return false;
}
+ bool isVectorIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID,
+ int ScalarOpdIdx) const {
+ return ScalarOpdIdx == -1;
+ }
+
/// Helper wrapper for the DemandedElts variant of getScalarizationOverhead.
InstructionCost getScalarizationOverhead(VectorType *InTy, bool Insert,
bool Extract,
diff --git a/llvm/lib/Analysis/TargetTransformInfo.cpp b/llvm/lib/Analysis/TargetTransformInfo.cpp
index a47462b61e03b2..f79348d27e78ff 100644
--- a/llvm/lib/Analysis/TargetTransformInfo.cpp
+++ b/llvm/lib/Analysis/TargetTransformInfo.cpp
@@ -612,6 +612,11 @@ bool TargetTransformInfo::isTargetIntrinsicWithScalarOpAtArg(
return TTIImpl->isTargetIntrinsicWithScalarOpAtArg(ID, ScalarOpdIdx);
}
+bool TargetTransformInfo::isVectorIntrinsicWithOverloadTypeAtArg(
+ Intrinsic::ID ID, int ScalarOpdIdx) const {
+ return TTIImpl->isVectorIntrinsicWithOverloadTypeAtArg(ID, ScalarOpdIdx);
+}
+
InstructionCost TargetTransformInfo::getScalarizationOverhead(
VectorType *Ty, const APInt &DemandedElts, bool Insert, bool Extract,
TTI::TargetCostKind CostKind) const {
diff --git a/llvm/lib/Target/DirectX/DirectXTargetTransformInfo.cpp b/llvm/lib/Target/DirectX/DirectXTargetTransformInfo.cpp
index 231afd8ae3eeaf..1c54887d9f56e2 100644
--- a/llvm/lib/Target/DirectX/DirectXTargetTransformInfo.cpp
+++ b/llvm/lib/Target/DirectX/DirectXTargetTransformInfo.cpp
@@ -25,6 +25,14 @@ bool DirectXTTIImpl::isTargetIntrinsicWithScalarOpAtArg(Intrinsic::ID ID,
}
}
+bool DirectXTTIImpl::isVectorIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID,
+ int ScalarOpdIdx) {
+ switch (ID) {
+ default:
+ return ScalarOpdIdx == -1;
+ }
+}
+
bool DirectXTTIImpl::isTargetIntrinsicTriviallyScalarizable(
Intrinsic::ID ID) const {
switch (ID) {
diff --git a/llvm/lib/Target/DirectX/DirectXTargetTransformInfo.h b/llvm/lib/Target/DirectX/DirectXTargetTransformInfo.h
index 30b57ed97d6370..a18e4a28625756 100644
--- a/llvm/lib/Target/DirectX/DirectXTargetTransformInfo.h
+++ b/llvm/lib/Target/DirectX/DirectXTargetTransformInfo.h
@@ -37,6 +37,8 @@ class DirectXTTIImpl : public BasicTTIImplBase<DirectXTTIImpl> {
bool isTargetIntrinsicTriviallyScalarizable(Intrinsic::ID ID) const;
bool isTargetIntrinsicWithScalarOpAtArg(Intrinsic::ID ID,
unsigned ScalarOpdIdx);
+ bool isVectorIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID,
+ int ScalarOpdIdx);
};
} // namespace llvm
diff --git a/llvm/lib/Transforms/Scalar/Scalarizer.cpp b/llvm/lib/Transforms/Scalar/Scalarizer.cpp
index 772f4c6c35ddec..1c61aa2712e8c7 100644
--- a/llvm/lib/Transforms/Scalar/Scalarizer.cpp
+++ b/llvm/lib/Transforms/Scalar/Scalarizer.cpp
@@ -280,6 +280,7 @@ class ScalarizerVisitor : public InstVisitor<ScalarizerVisitor, bool> {
bool visit(Function &F);
bool isTriviallyScalarizable(Intrinsic::ID ID);
+ bool isIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID, int ScalarOpdIdx);
// InstVisitor methods. They return true if the instruction was scalarized,
// false if nothing changed.
@@ -696,6 +697,13 @@ bool ScalarizerVisitor::isTriviallyScalarizable(Intrinsic::ID ID) {
TTI->isTargetIntrinsicTriviallyScalarizable(ID);
}
+bool ScalarizerVisitor::isIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID,
+ int ScalarOpdIdx) {
+ return Intrinsic::isTargetIntrinsic(ID)
+ ? TTI->isVectorIntrinsicWithOverloadTypeAtArg(ID, ScalarOpdIdx)
+ : isVectorIntrinsicWithOverloadTypeAtArg(ID, ScalarOpdIdx);
+}
+
/// If a call to a vector typed intrinsic function, split into a scalar call per
/// element if possible for the intrinsic.
bool ScalarizerVisitor::splitCall(CallInst &CI) {
@@ -727,7 +735,7 @@ bool ScalarizerVisitor::splitCall(CallInst &CI) {
SmallVector<llvm::Type *, 3> Tys;
// Add return type if intrinsic is overloaded on it.
- if (isVectorIntrinsicWithOverloadTypeAtArg(ID, -1))
+ if (isIntrinsicWithOverloadTypeAtArg(ID, -1))
Tys.push_back(VS->SplitTy);
if (AreAllVectorsOfMatchingSize) {
@@ -767,13 +775,13 @@ bool ScalarizerVisitor::splitCall(CallInst &CI) {
}
Scattered[I] = scatter(&CI, OpI, *OpVS);
- if (isVectorIntrinsicWithOverloadTypeAtArg(ID, I)) {
+ if (isIntrinsicWithOverloadTypeAtArg(ID, I)) {
OverloadIdx[I] = Tys.size();
Tys.push_back(OpVS->SplitTy);
}
} else {
ScalarOperands[I] = OpI;
- if (isVectorIntrinsicWithOverloadTypeAtArg(ID, I))
+ if (isIntrinsicWithOverloadTypeAtArg(ID, I))
Tys.push_back(OpI->getType());
}
}
|
There was a problem hiding this comment.
This feels like a partial change which leaves us in an odd state.
Instead of cleaning up all the uses of the VectorUtils isVectorIntrinsicWithOverloadTypeAtArg, you only updated the uses in the Scalarizer, which in turn makes the Scalarizer's implementation a bit odd.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Hmm, okay. Are you suggesting that we should replace all the uses or that we can't do such a change? Having looked at the other uses of |
@llvm-beanz I think I misunderstood what you were asking. We aren't implementing the TTI for all of |
I'm also unsure how this relates to my comment. This change partially moves It seems to me that we should have one way to query (cc @bogner) |
- consolidate the usage of isVectorIntrinsicWithOverloadTypeAtArg into one place of VectorUtils.cppp
| @@ -152,7 +152,8 @@ bool isVectorIntrinsicWithScalarOpAtArg(Intrinsic::ID ID, | |||
|
|
|||
| /// Identifies if the vector form of the intrinsic is overloaded on the type of | |||
| /// the operand at index \p OpdIdx, or on the return type if \p OpdIdx is -1. | |||
| bool isVectorIntrinsicWithOverloadTypeAtArg(Intrinsic::ID ID, int OpdIdx); | |||
| bool isVectorIntrinsicWithOverloadTypeAtArg( | |||
| Intrinsic::ID ID, int OpdIdx, const TargetTransformInfo *TTI = nullptr); | |||
There was a problem hiding this comment.
Could also do with an std::optional pointer here, not sure on what is preferred.
There was a problem hiding this comment.
I don't think this should be optional. In any case where this argument matters it would be a bug to not pass it.
|
We really should be updating the other users of the As for updating the vectorizer uses:
|
Does it make sense to make a few issues for this and let this pr go in as is? I started this pattern with some of the other isVectorIntrinsic functions so feels like the cleanup should cover all the functions. |
It seems clear to me that we should file issues to fix the other cases, but I don't see why we would merge this one, then just have to clean it up. This is an NFC change, having the NFC refactoring end up in the correct end state seems better to me. |
- this pass is used to replace a vectorizable intrinsic function with a generic veclib function - we make the assumption that a target specific intrinisc is not going to be replaced with a generic function call and so we don't need to provide the TTI
- use the already defined TTI as the argument
- pass down the TTI from the LoopVectorizationPlanner into the VPTransformState - this will allow this information to all VPRecipe::execute functions when they generate new IR
- use the updated VPTransformState to pass down the TTI
| @@ -110,15 +110,17 @@ static bool replaceWithCallToVeclib(const TargetLibraryInfo &TLI, | |||
|
|
|||
| // OloadTys collects types used in scalar intrinsic overload name. | |||
| SmallVector<Type *, 3> OloadTys; | |||
| if (!RetTy->isVoidTy() && isVectorIntrinsicWithOverloadTypeAtArg(IID, -1)) | |||
| if (!RetTy->isVoidTy() && | |||
| isVectorIntrinsicWithOverloadTypeAtArg(IID, -1, /*TTI=*/nullptr)) | |||
There was a problem hiding this comment.
Regarding adding a dependency on the TargetTransformInfo analysis pass to retrieve TTI. There is currently no intrinsic that would execute and use TTI in isVectorIntrinsicWithOverloadTypeAtArg. While I don't expect the pass is expensive to do, it is currently completely unnecessary work. So I think it is okay to defer until there is an actual use case. Which as mentioned, seems unlikely.
| switch (ID) { | ||
| default: | ||
| return ScalarOpdIdx == -1; | ||
| } |
There was a problem hiding this comment.
In case anyone else wonders why the switch statement is here, observe the follow-on in #114847
| @@ -152,7 +152,8 @@ bool isVectorIntrinsicWithScalarOpAtArg(Intrinsic::ID ID, | |||
|
|
|||
| /// Identifies if the vector form of the intrinsic is overloaded on the type of | |||
| /// the operand at index \p OpdIdx, or on the return type if \p OpdIdx is -1. | |||
There was a problem hiding this comment.
You may wish to update this comment to mention the new parameter. As many places as this function is implemented at different levels and as confusing as I initially found the name of it, it might be worth copying this to at least a few of the places that it and it's facilitators are declared/defined.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This changes allows target intrinsic to specify and overwrite overloaded types.
This change will let us add scalarization for
asdouble: #114847