[CodeGen] Forbid passing a PointerType to MVT::getVT and EVT::getEVT #92671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
You can test this locally with the following command:git-clang-format --diff 7529fe2e92e79eef22a528a7168e4dd777d6e9bd 7fb0c673f38c0ff8913aff811b3b3bcac74b48d1 -- llvm/include/llvm/CodeGen/ValueTypes.h llvm/include/llvm/CodeGenTypes/MachineValueType.h llvm/lib/CodeGen/ValueTypes.cppView the diff from clang-format here.diff --git a/llvm/lib/CodeGen/ValueTypes.cpp b/llvm/lib/CodeGen/ValueTypes.cpp
index 3d5c58d282..74d0128f84 100644
--- a/llvm/lib/CodeGen/ValueTypes.cpp
+++ b/llvm/lib/CodeGen/ValueTypes.cpp
@@ -612,7 +612,8 @@ MVT MVT::getVT(Type *Ty, bool HandleUnknown){
}
case Type::X86_AMXTyID: return MVT(MVT::x86amx);
case Type::FP128TyID: return MVT(MVT::f128);
- case Type::PPC_FP128TyID: return MVT(MVT::ppcf128);
+ case Type::PPC_FP128TyID:
+ return MVT(MVT::ppcf128);
case Type::FixedVectorTyID:
case Type::ScalableVectorTyID: {
VectorType *VTy = cast<VectorType>(Ty);
|
jrtc27
force-pushed
the
mvt-getvt-forbid-pointer
branch
from
ed5bc5c
to
c66ee00
Compare
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-llvm-selectiondag Author: Jessica Clarke (jrtc27) ChangesThere is the expectation throughout CodeGen that, for types representing "real" values, the MVT or EVT is self-contained. However, MVT::iPTR is challenging, because it has no address space, and even if it did, there often is no DataLayout immediately accessible to determine what actually is the underlying type. Historically it was documented as being TableGen-only, but that was lost in 631bfdb's conversion to using the generated defines. Let's preserve that intent by not allowing it to originate through accidental calls to get(E)VT with a PointerType. If you need to support that, be sure to use something like TargetLowering's getValueType, which takes a DataLayout and can map pointers to their concrete MVTs. Full diff: https://github.com/llvm/llvm-project/pull/92671.diff 3 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/ValueTypes.h b/llvm/include/llvm/CodeGen/ValueTypes.h
index b66c66d1bfc45..a4d57fba7a7a0 100644
--- a/llvm/include/llvm/CodeGen/ValueTypes.h
+++ b/llvm/include/llvm/CodeGen/ValueTypes.h
@@ -488,8 +488,8 @@ namespace llvm {
Type *getTypeForEVT(LLVMContext &Context) const;
/// Return the value type corresponding to the specified type.
- /// This returns all pointers as iPTR. If HandleUnknown is true, unknown
- /// types are returned as Other, otherwise they are invalid.
+ /// If HandleUnknown is true, unknown types are returned as Other,
+ /// otherwise they are invalid.
static EVT getEVT(Type *Ty, bool HandleUnknown = false);
intptr_t getRawBits() const {
diff --git a/llvm/include/llvm/CodeGenTypes/MachineValueType.h b/llvm/include/llvm/CodeGenTypes/MachineValueType.h
index 9aceb9896021c..f3280d0276399 100644
--- a/llvm/include/llvm/CodeGenTypes/MachineValueType.h
+++ b/llvm/include/llvm/CodeGenTypes/MachineValueType.h
@@ -476,9 +476,9 @@ namespace llvm {
return getVectorVT(VT, EC.getKnownMinValue());
}
- /// Return the value type corresponding to the specified type. This returns
- /// all pointers as iPTR. If HandleUnknown is true, unknown types are
- /// returned as Other, otherwise they are invalid.
+ /// Return the value type corresponding to the specified type.
+ /// If HandleUnknown is true, unknown types are returned as Other,
+ /// otherwise they are invalid.
static MVT getVT(Type *Ty, bool HandleUnknown = false);
public:
diff --git a/llvm/lib/CodeGen/ValueTypes.cpp b/llvm/lib/CodeGen/ValueTypes.cpp
index 58db686ec7d57..38d2438f76b9d 100644
--- a/llvm/lib/CodeGen/ValueTypes.cpp
+++ b/llvm/lib/CodeGen/ValueTypes.cpp
@@ -579,9 +579,9 @@ Type *EVT::getTypeForEVT(LLVMContext &Context) const {
// clang-format on
}
-/// Return the value type corresponding to the specified type. This returns all
-/// pointers as MVT::iPTR. If HandleUnknown is true, unknown types are returned
-/// as Other, otherwise they are invalid.
+/// Return the value type corresponding to the specified type.
+/// If HandleUnknown is true, unknown types are returned as Other, otherwise
+/// they are invalid.
MVT MVT::getVT(Type *Ty, bool HandleUnknown){
assert(Ty != nullptr && "Invalid type");
switch (Ty->getTypeID()) {
@@ -611,7 +611,6 @@ MVT MVT::getVT(Type *Ty, bool HandleUnknown){
case Type::X86_AMXTyID: return MVT(MVT::x86amx);
case Type::FP128TyID: return MVT(MVT::f128);
case Type::PPC_FP128TyID: return MVT(MVT::ppcf128);
- case Type::PointerTyID: return MVT(MVT::iPTR);
case Type::FixedVectorTyID:
case Type::ScalableVectorTyID: {
VectorType *VTy = cast<VectorType>(Ty);
@@ -622,9 +621,9 @@ MVT MVT::getVT(Type *Ty, bool HandleUnknown){
}
}
-/// getEVT - Return the value type corresponding to the specified type. This
-/// returns all pointers as MVT::iPTR. If HandleUnknown is true, unknown types
-/// are returned as Other, otherwise they are invalid.
+/// getEVT - Return the value type corresponding to the specified type.
+/// If HandleUnknown is true, unknown types are returned as Other, otherwise
+/// they are invalid.
EVT EVT::getEVT(Type *Ty, bool HandleUnknown){
switch (Ty->getTypeID()) {
default:
|
arsenm
approved these changes
May 22, 2024
| /// This returns all pointers as iPTR. If HandleUnknown is true, unknown | ||
| /// types are returned as Other, otherwise they are invalid. | ||
| /// If HandleUnknown is true, unknown types are returned as Other, | ||
| /// otherwise they are invalid. |
There was a problem hiding this comment.
Maybe should make a note about pointer handling?
There was a problem hiding this comment.
Sure. I should also add to the comment in MachineValueTypes.td like used to be in the .h.
There was a problem hiding this comment.
Comments updated, including ValueTypes.td itself
jrtc27
force-pushed
the
mvt-getvt-forbid-pointer
branch
from
c66ee00
to
cc87f9a
Compare
There is the expectation throughout CodeGen that, for types representing "real" values, the MVT or EVT is self-contained. However, MVT::iPTR is challenging, because it has no address space, and even if it did, there often is no DataLayout immediately accessible to determine what actually is the underlying type. Historically it was documented as being TableGen-only, but that was lost in 631bfdb's conversion to using the generated defines. Let's preserve that intent by not allowing it to originate through accidental calls to get(E)VT with a PointerType. If you need to support that, be sure to use something like TargetLowering's getValueType, which takes a DataLayout and can map pointers to their concrete MVTs. Whilst here, reintroduce documentation about these value types being TableGen-only.
jrtc27
force-pushed
the
mvt-getvt-forbid-pointer
branch
from
cc87f9a
to
7fb0c67
Compare
arsenm
approved these changes
May 22, 2024
|
I believe this broke the build of miniQMC with the following assertion. |
jplehr
added a commit
to jplehr/llvm-project
that referenced
this pull request
May 27, 2024
This is a temporary workaround for the build issues we are observing after llvm#92671. It re-enables the handling of pointer types by simply passing the HandleUnknown flag to getVT set to true whenever a PointerTy is encountered.
jplehr
added a commit
to jplehr/llvm-project
that referenced
this pull request
May 27, 2024
This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info and work around a build issue we were seeing for the miniQMC application after llvm#92671.
jplehr
added a commit
to jplehr/llvm-project
that referenced
this pull request
May 28, 2024
This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info and work around a build issue we were seeing for the miniQMC application after llvm#92671. The test is reduced from that specific application and reproduces the issue with the loop-vectorizer pass. However, to me, it seemed that the issue was coming from the cost model.
jplehr
added a commit
to jplehr/llvm-project
that referenced
this pull request
May 28, 2024
This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info and work around a build issue we were seeing for the miniQMC application after llvm#92671. The test is reduced from that specific application and reproduces the issue with the loop-vectorizer pass. However, to me, it seemed that the issue was coming from the cost model.
jplehr
added a commit
to jplehr/llvm-project
that referenced
this pull request
May 28, 2024
This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info and work around a build issue we were seeing for the miniQMC application after llvm#92671. The test is reduced from that specific application and reproduces the issue with the loop-vectorizer pass. However, to me, it seemed that the issue was coming from the cost model.
jplehr
added a commit
that referenced
this pull request
May 29, 2024
This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info inside the X86 cost model. This resolves a build issue we were seeing for the miniQMC application after #92671.
jameshu15869
pushed a commit
to jameshu15869/llvm-project
that referenced
this pull request
May 31, 2024
…lvm#92671) There is the expectation throughout CodeGen that, for types representing "real" values, the MVT or EVT is self-contained. However, MVT::iPTR is challenging, because it has no address space, and even if it did, there often is no DataLayout immediately accessible to determine what actually is the underlying type. Historically it was documented as being TableGen-only, but that was lost in 631bfdb's conversion to using the generated defines. Let's preserve that intent by not allowing it to originate through accidental calls to get(E)VT with a PointerType. If you need to support that, be sure to use something like TargetLowering's getValueType, which takes a DataLayout and can map pointers to their concrete MVTs. Whilst here, reintroduce documentation about these value types being TableGen-only.
jameshu15869
pushed a commit
to jameshu15869/llvm-project
that referenced
this pull request
May 31, 2024
This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info inside the X86 cost model. This resolves a build issue we were seeing for the miniQMC application after llvm#92671.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is the expectation throughout CodeGen that, for types representing "real" values, the MVT or EVT is self-contained. However, MVT::iPTR is challenging, because it has no address space, and even if it did, there often is no DataLayout immediately accessible to determine what actually is the underlying type.
Historically it was documented as being TableGen-only, but that was lost in 631bfdb's conversion to using the generated defines. Let's preserve that intent by not allowing it to originate through accidental calls to get(E)VT with a PointerType. If you need to support that, be sure to use something like TargetLowering's getValueType, which takes a DataLayout and can map pointers to their concrete MVTs. Whilst here, reintroduce documentation about these value types being TableGen-only.