[IR] Fix undiagnosed cases of structs containing scalable vectors #113455
Conversation
This file contains hidden or 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
Member
|
@llvm/pr-subscribers-llvm-ir Author: Jay Foad (jayfoad) ChangesType::isScalableTy and StructType::containsScalableVectorType failed to Full diff: https://github.com/llvm/llvm-project/pull/113455.diff 7 Files Affected:
diff --git a/llvm/include/llvm/IR/DerivedTypes.h b/llvm/include/llvm/IR/DerivedTypes.h
index a24801d8bdf834..820b5c0707df6c 100644
--- a/llvm/include/llvm/IR/DerivedTypes.h
+++ b/llvm/include/llvm/IR/DerivedTypes.h
@@ -290,8 +290,8 @@ class StructType : public Type {
bool isSized(SmallPtrSetImpl<Type *> *Visited = nullptr) const;
/// Returns true if this struct contains a scalable vector.
- bool
- containsScalableVectorType(SmallPtrSetImpl<Type *> *Visited = nullptr) const;
+ bool isScalableTy(SmallPtrSetImpl<const Type *> &Visited) const;
+ using Type::isScalableTy;
/// Returns true if this struct contains homogeneous scalable vector types.
/// Note that the definition of homogeneous scalable vector type is not
diff --git a/llvm/include/llvm/IR/Type.h b/llvm/include/llvm/IR/Type.h
index 2f53197df19998..d563b25d600a0c 100644
--- a/llvm/include/llvm/IR/Type.h
+++ b/llvm/include/llvm/IR/Type.h
@@ -206,6 +206,7 @@ class Type {
bool isScalableTargetExtTy() const;
/// Return true if this is a type whose size is a known multiple of vscale.
+ bool isScalableTy(SmallPtrSetImpl<const Type *> &Visited) const;
bool isScalableTy() const;
/// Return true if this is a FP type or a vector of FP.
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 6a2372c9751408..8ddb2efb0e26c2 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -8525,7 +8525,7 @@ int LLParser::parseGetElementPtr(Instruction *&Inst, PerFunctionState &PFS) {
return error(Loc, "base element of getelementptr must be sized");
auto *STy = dyn_cast<StructType>(Ty);
- if (STy && STy->containsScalableVectorType())
+ if (STy && STy->isScalableTy())
return error(Loc, "getelementptr cannot target structure that contains "
"scalable vector type");
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index f618263f79c313..912b1a3960ef19 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -58,16 +58,19 @@ bool Type::isIntegerTy(unsigned Bitwidth) const {
return isIntegerTy() && cast<IntegerType>(this)->getBitWidth() == Bitwidth;
}
-bool Type::isScalableTy() const {
+bool Type::isScalableTy(SmallPtrSetImpl<const Type *> &Visited) const {
if (const auto *ATy = dyn_cast<ArrayType>(this))
- return ATy->getElementType()->isScalableTy();
- if (const auto *STy = dyn_cast<StructType>(this)) {
- SmallPtrSet<Type *, 4> Visited;
- return STy->containsScalableVectorType(&Visited);
- }
+ return ATy->getElementType()->isScalableTy(Visited);
+ if (const auto *STy = dyn_cast<StructType>(this))
+ return STy->isScalableTy(Visited);
return getTypeID() == ScalableVectorTyID || isScalableTargetExtTy();
}
+bool Type::isScalableTy() const {
+ SmallPtrSet<const Type *, 4> Visited;
+ return isScalableTy(Visited);
+}
+
const fltSemantics &Type::getFltSemantics() const {
switch (getTypeID()) {
case HalfTyID: return APFloat::IEEEhalf();
@@ -394,30 +397,22 @@ StructType *StructType::get(LLVMContext &Context, ArrayRef<Type*> ETypes,
return ST;
}
-bool StructType::containsScalableVectorType(
- SmallPtrSetImpl<Type *> *Visited) const {
+bool StructType::isScalableTy(SmallPtrSetImpl<const Type *> &Visited) const {
if ((getSubclassData() & SCDB_ContainsScalableVector) != 0)
return true;
if ((getSubclassData() & SCDB_NotContainsScalableVector) != 0)
return false;
- if (Visited && !Visited->insert(const_cast<StructType *>(this)).second)
+ if (!Visited.insert(this).second)
return false;
for (Type *Ty : elements()) {
- if (isa<ScalableVectorType>(Ty)) {
+ if (Ty->isScalableTy(Visited)) {
const_cast<StructType *>(this)->setSubclassData(
getSubclassData() | SCDB_ContainsScalableVector);
return true;
}
- if (auto *STy = dyn_cast<StructType>(Ty)) {
- if (STy->containsScalableVectorType(Visited)) {
- const_cast<StructType *>(this)->setSubclassData(
- getSubclassData() | SCDB_ContainsScalableVector);
- return true;
- }
- }
}
// For structures that are opaque, return false but do not set the
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index f34fe7594c8602..60e65392218dad 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -4107,8 +4107,7 @@ void Verifier::visitGetElementPtrInst(GetElementPtrInst &GEP) {
Check(GEP.getSourceElementType()->isSized(), "GEP into unsized type!", &GEP);
if (auto *STy = dyn_cast<StructType>(GEP.getSourceElementType())) {
- SmallPtrSet<Type *, 4> Visited;
- Check(!STy->containsScalableVectorType(&Visited),
+ Check(!STy->isScalableTy(),
"getelementptr cannot target structure that contains scalable vector"
"type",
&GEP);
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index c8b9f166b16020..971ace2a4f4716 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -4087,7 +4087,7 @@ Instruction *InstCombinerImpl::visitExtractValueInst(ExtractValueInst &EV) {
if (LoadInst *L = dyn_cast<LoadInst>(Agg)) {
// Bail out if the aggregate contains scalable vector type
if (auto *STy = dyn_cast<StructType>(Agg->getType());
- STy && STy->containsScalableVectorType())
+ STy && STy->isScalableTy())
return nullptr;
// If the (non-volatile) load only has one use, we can rewrite this to a
diff --git a/llvm/test/Verifier/scalable-global-vars.ll b/llvm/test/Verifier/scalable-global-vars.ll
index 81882261e664ef..fb9a3067acba98 100644
--- a/llvm/test/Verifier/scalable-global-vars.ll
+++ b/llvm/test/Verifier/scalable-global-vars.ll
@@ -15,3 +15,17 @@
; CHECK-NEXT: ptr @ScalableVecStructGlobal
@ScalableVecStructGlobal = global { i32, <vscale x 4 x i32> } zeroinitializer
+; CHECK-NEXT: Globals cannot contain scalable types
+; CHECK-NEXT: ptr @StructTestGlobal
+%struct.test = type { <vscale x 1 x double>, <vscale x 1 x double> }
+@StructTestGlobal = global %struct.test zeroinitializer
+
+; CHECK-NEXT: Globals cannot contain scalable types
+; CHECK-NEXT: ptr @StructArrayTestGlobal
+%struct.array.test = type { [2 x <vscale x 1 x double>] }
+@StructArrayTestGlobal = global %struct.array.test zeroinitializer
+
+; CHECK-NEXT: Globals cannot contain scalable types
+; CHECK-NEXT: ptr @StructTargetTestGlobal
+%struct.target.test = type { target("aarch64.svcount"), target("aarch64.svcount") }
+@StructTargetTestGlobal = global %struct.target.test zeroinitializer
|
Member
|
@llvm/pr-subscribers-llvm-transforms Author: Jay Foad (jayfoad) ChangesType::isScalableTy and StructType::containsScalableVectorType failed to Full diff: https://github.com/llvm/llvm-project/pull/113455.diff 7 Files Affected:
diff --git a/llvm/include/llvm/IR/DerivedTypes.h b/llvm/include/llvm/IR/DerivedTypes.h
index a24801d8bdf834..820b5c0707df6c 100644
--- a/llvm/include/llvm/IR/DerivedTypes.h
+++ b/llvm/include/llvm/IR/DerivedTypes.h
@@ -290,8 +290,8 @@ class StructType : public Type {
bool isSized(SmallPtrSetImpl<Type *> *Visited = nullptr) const;
/// Returns true if this struct contains a scalable vector.
- bool
- containsScalableVectorType(SmallPtrSetImpl<Type *> *Visited = nullptr) const;
+ bool isScalableTy(SmallPtrSetImpl<const Type *> &Visited) const;
+ using Type::isScalableTy;
/// Returns true if this struct contains homogeneous scalable vector types.
/// Note that the definition of homogeneous scalable vector type is not
diff --git a/llvm/include/llvm/IR/Type.h b/llvm/include/llvm/IR/Type.h
index 2f53197df19998..d563b25d600a0c 100644
--- a/llvm/include/llvm/IR/Type.h
+++ b/llvm/include/llvm/IR/Type.h
@@ -206,6 +206,7 @@ class Type {
bool isScalableTargetExtTy() const;
/// Return true if this is a type whose size is a known multiple of vscale.
+ bool isScalableTy(SmallPtrSetImpl<const Type *> &Visited) const;
bool isScalableTy() const;
/// Return true if this is a FP type or a vector of FP.
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 6a2372c9751408..8ddb2efb0e26c2 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -8525,7 +8525,7 @@ int LLParser::parseGetElementPtr(Instruction *&Inst, PerFunctionState &PFS) {
return error(Loc, "base element of getelementptr must be sized");
auto *STy = dyn_cast<StructType>(Ty);
- if (STy && STy->containsScalableVectorType())
+ if (STy && STy->isScalableTy())
return error(Loc, "getelementptr cannot target structure that contains "
"scalable vector type");
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index f618263f79c313..912b1a3960ef19 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -58,16 +58,19 @@ bool Type::isIntegerTy(unsigned Bitwidth) const {
return isIntegerTy() && cast<IntegerType>(this)->getBitWidth() == Bitwidth;
}
-bool Type::isScalableTy() const {
+bool Type::isScalableTy(SmallPtrSetImpl<const Type *> &Visited) const {
if (const auto *ATy = dyn_cast<ArrayType>(this))
- return ATy->getElementType()->isScalableTy();
- if (const auto *STy = dyn_cast<StructType>(this)) {
- SmallPtrSet<Type *, 4> Visited;
- return STy->containsScalableVectorType(&Visited);
- }
+ return ATy->getElementType()->isScalableTy(Visited);
+ if (const auto *STy = dyn_cast<StructType>(this))
+ return STy->isScalableTy(Visited);
return getTypeID() == ScalableVectorTyID || isScalableTargetExtTy();
}
+bool Type::isScalableTy() const {
+ SmallPtrSet<const Type *, 4> Visited;
+ return isScalableTy(Visited);
+}
+
const fltSemantics &Type::getFltSemantics() const {
switch (getTypeID()) {
case HalfTyID: return APFloat::IEEEhalf();
@@ -394,30 +397,22 @@ StructType *StructType::get(LLVMContext &Context, ArrayRef<Type*> ETypes,
return ST;
}
-bool StructType::containsScalableVectorType(
- SmallPtrSetImpl<Type *> *Visited) const {
+bool StructType::isScalableTy(SmallPtrSetImpl<const Type *> &Visited) const {
if ((getSubclassData() & SCDB_ContainsScalableVector) != 0)
return true;
if ((getSubclassData() & SCDB_NotContainsScalableVector) != 0)
return false;
- if (Visited && !Visited->insert(const_cast<StructType *>(this)).second)
+ if (!Visited.insert(this).second)
return false;
for (Type *Ty : elements()) {
- if (isa<ScalableVectorType>(Ty)) {
+ if (Ty->isScalableTy(Visited)) {
const_cast<StructType *>(this)->setSubclassData(
getSubclassData() | SCDB_ContainsScalableVector);
return true;
}
- if (auto *STy = dyn_cast<StructType>(Ty)) {
- if (STy->containsScalableVectorType(Visited)) {
- const_cast<StructType *>(this)->setSubclassData(
- getSubclassData() | SCDB_ContainsScalableVector);
- return true;
- }
- }
}
// For structures that are opaque, return false but do not set the
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index f34fe7594c8602..60e65392218dad 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -4107,8 +4107,7 @@ void Verifier::visitGetElementPtrInst(GetElementPtrInst &GEP) {
Check(GEP.getSourceElementType()->isSized(), "GEP into unsized type!", &GEP);
if (auto *STy = dyn_cast<StructType>(GEP.getSourceElementType())) {
- SmallPtrSet<Type *, 4> Visited;
- Check(!STy->containsScalableVectorType(&Visited),
+ Check(!STy->isScalableTy(),
"getelementptr cannot target structure that contains scalable vector"
"type",
&GEP);
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index c8b9f166b16020..971ace2a4f4716 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -4087,7 +4087,7 @@ Instruction *InstCombinerImpl::visitExtractValueInst(ExtractValueInst &EV) {
if (LoadInst *L = dyn_cast<LoadInst>(Agg)) {
// Bail out if the aggregate contains scalable vector type
if (auto *STy = dyn_cast<StructType>(Agg->getType());
- STy && STy->containsScalableVectorType())
+ STy && STy->isScalableTy())
return nullptr;
// If the (non-volatile) load only has one use, we can rewrite this to a
diff --git a/llvm/test/Verifier/scalable-global-vars.ll b/llvm/test/Verifier/scalable-global-vars.ll
index 81882261e664ef..fb9a3067acba98 100644
--- a/llvm/test/Verifier/scalable-global-vars.ll
+++ b/llvm/test/Verifier/scalable-global-vars.ll
@@ -15,3 +15,17 @@
; CHECK-NEXT: ptr @ScalableVecStructGlobal
@ScalableVecStructGlobal = global { i32, <vscale x 4 x i32> } zeroinitializer
+; CHECK-NEXT: Globals cannot contain scalable types
+; CHECK-NEXT: ptr @StructTestGlobal
+%struct.test = type { <vscale x 1 x double>, <vscale x 1 x double> }
+@StructTestGlobal = global %struct.test zeroinitializer
+
+; CHECK-NEXT: Globals cannot contain scalable types
+; CHECK-NEXT: ptr @StructArrayTestGlobal
+%struct.array.test = type { [2 x <vscale x 1 x double>] }
+@StructArrayTestGlobal = global %struct.array.test zeroinitializer
+
+; CHECK-NEXT: Globals cannot contain scalable types
+; CHECK-NEXT: ptr @StructTargetTestGlobal
+%struct.target.test = type { target("aarch64.svcount"), target("aarch64.svcount") }
+@StructTargetTestGlobal = global %struct.target.test zeroinitializer
|
jayfoad
requested review from
4vtomat,
christetreault-llvm,
david-arm,
efriedma-quic,
eopXD,
huntergr-arm,
paulwalker-arm,
sdesmalen-arm and
topperc
Contributor
Author
|
The commits are intended to be individually reviewable if you prefer to do that. |
Member
|
Thanks for the fix! Just curious about what kind of test would fail? |
Contributor
Author
The last two cases that I added to |
paulwalker-arm
approved these changes
Oct 25, 2024
huntergr-arm
approved these changes
Oct 25, 2024
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/1018 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/6673 Here is the relevant piece of the build log for the reference |
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
…vm#113455) Type::isScalableTy and StructType::containsScalableVectorType failed to detect some cases of structs containing scalable vectors because containsScalableVectorType did not call back into isScalableTy to check the element types. Fix this, which requires sharing the same Visited set in both functions. Also change the external API so that callers are never required to pass in a Visited set, and normalize the naming to isScalableTy.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Type::isScalableTy and StructType::containsScalableVectorType failed to
detect some cases of structs containing scalable vectors because
containsScalableVectorType did not call back into isScalableTy to check
the element types. Fix this, which requires sharing the same Visited set
in both functions. Also change the external API so that callers are
never required to pass in a Visited set, and normalize the naming to
isScalableTy.