[llvm] [IR] Check parameters of target extension types on construction (PR #107268)
Jay Foad via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 5 07:41:54 PDT 2024
https://github.com/jayfoad updated https://github.com/llvm/llvm-project/pull/107268
>From b978ec320c7869c634e84d79616fce2b55599fa1 Mon Sep 17 00:00:00 2001
From: Jay Foad <jay.foad at amd.com>
Date: Wed, 4 Sep 2024 12:50:46 +0100
Subject: [PATCH 1/6] [IR] Check parameters of target extension types on
construction
Since IR Types are immutable it makes sense to check them on
construction instead of in the IR Verifier pass.
This patch checks that some TargetExtTypes are well-formed in the sense
that they have the expected number of type parameters and integer
parameters. When called from LLParser it gives a diagnostic message.
When called from anywhere else it just asserts that they are
well-formed.
---
llvm/include/llvm/IR/DerivedTypes.h | 14 +++++++
llvm/lib/AsmParser/LLParser.cpp | 7 +++-
llvm/lib/IR/Type.cpp | 37 +++++++++++++++----
.../Assembler/target-type-param-errors.ll | 12 ++++++
4 files changed, 62 insertions(+), 8 deletions(-)
create mode 100644 llvm/test/Assembler/target-type-param-errors.ll
diff --git a/llvm/include/llvm/IR/DerivedTypes.h b/llvm/include/llvm/IR/DerivedTypes.h
index 01f76d49327808..0c8cbe1921ac9b 100644
--- a/llvm/include/llvm/IR/DerivedTypes.h
+++ b/llvm/include/llvm/IR/DerivedTypes.h
@@ -32,6 +32,7 @@ namespace llvm {
class Value;
class APInt;
class LLVMContext;
+template <typename T> class Expected;
/// Class to represent integer types. Note that this class is also used to
/// represent the built-in integer types: Int1Ty, Int8Ty, Int16Ty, Int32Ty and
@@ -735,6 +736,19 @@ class TargetExtType : public Type {
ArrayRef<Type *> Types = std::nullopt,
ArrayRef<unsigned> Ints = std::nullopt);
+ /// Return a target extension type having the specified name and optional
+ /// type and integer parameters, or an appropriate Error if it fails the
+ /// parameters check.
+ static Expected<TargetExtType *>
+ getOrError(LLVMContext &Context, StringRef Name,
+ ArrayRef<Type *> Types = std::nullopt,
+ ArrayRef<unsigned> Ints = std::nullopt);
+
+ /// Check that a newly created target extension type has the expected number
+ /// of type parameters and integer parameters, returning the type itself if OK
+ /// or an appropriate Error if not.
+ static Expected<TargetExtType *> checkParams(TargetExtType *TTy);
+
/// Return the name for this target extension type. Two distinct target
/// extension types may have the same name if their type or integer parameters
/// differ.
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index f41907f0351257..93dc2bd241581b 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -3530,7 +3530,12 @@ bool LLParser::parseTargetExtType(Type *&Result) {
if (parseToken(lltok::rparen, "expected ')' in target extension type"))
return true;
- Result = TargetExtType::get(Context, TypeName, TypeParams, IntParams);
+ auto TTy =
+ TargetExtType::getOrError(Context, TypeName, TypeParams, IntParams);
+ if (auto E = TTy.takeError())
+ return tokError(toString(std::move(E)));
+
+ Result = *TTy;
return false;
}
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index 753aa5fd2118e5..0bb9b282385ac7 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -22,6 +22,7 @@
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/Value.h"
#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/TypeSize.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/TargetParser/RISCVTargetParser.h"
@@ -792,6 +793,13 @@ TargetExtType::TargetExtType(LLVMContext &C, StringRef Name,
TargetExtType *TargetExtType::get(LLVMContext &C, StringRef Name,
ArrayRef<Type *> Types,
ArrayRef<unsigned> Ints) {
+ return cantFail(getOrError(C, Name, Types, Ints));
+}
+
+Expected<TargetExtType *> TargetExtType::getOrError(LLVMContext &C,
+ StringRef Name,
+ ArrayRef<Type *> Types,
+ ArrayRef<unsigned> Ints) {
const TargetExtTypeKeyInfo::KeyTy Key(Name, Types, Ints);
TargetExtType *TT;
// Since we only want to allocate a fresh target type in case none is found
@@ -799,8 +807,8 @@ TargetExtType *TargetExtType::get(LLVMContext &C, StringRef Name,
// one for inserting the newly allocated one), here we instead lookup based on
// Key and update the reference to the target type in-place to a newly
// allocated one if not found.
- auto Insertion = C.pImpl->TargetExtTypes.insert_as(nullptr, Key);
- if (Insertion.second) {
+ auto [Iter, Inserted] = C.pImpl->TargetExtTypes.insert_as(nullptr, Key);
+ if (Inserted) {
// The target type was not found. Allocate one and update TargetExtTypes
// in-place.
TT = (TargetExtType *)C.pImpl->Alloc.Allocate(
@@ -808,12 +816,27 @@ TargetExtType *TargetExtType::get(LLVMContext &C, StringRef Name,
sizeof(unsigned) * Ints.size(),
alignof(TargetExtType));
new (TT) TargetExtType(C, Name, Types, Ints);
- *Insertion.first = TT;
- } else {
- // The target type was found. Just return it.
- TT = *Insertion.first;
+ *Iter = TT;
+ return checkParams(TT);
}
- return TT;
+
+ // The target type was found. Just return it.
+ return *Iter;
+}
+
+Expected<TargetExtType *> TargetExtType::checkParams(TargetExtType *TTy) {
+ // Opaque types in the AArch64 name space.
+ if (TTy->Name == "aarch64.svcount" &&
+ (TTy->getNumTypeParameters() != 0 || TTy->getNumIntParameters() != 0))
+ return createStringError("Target type should have no parameters");
+
+ // Opaque types in the RISC-V name space.
+ if (TTy->Name == "riscv.vector.tuple" &&
+ (TTy->getNumTypeParameters() != 1 || TTy->getNumIntParameters() != 1))
+ return createStringError(
+ "Target type should have one type parameter and one integer parameter");
+
+ return TTy;
}
namespace {
diff --git a/llvm/test/Assembler/target-type-param-errors.ll b/llvm/test/Assembler/target-type-param-errors.ll
new file mode 100644
index 00000000000000..a356a0804fafa7
--- /dev/null
+++ b/llvm/test/Assembler/target-type-param-errors.ll
@@ -0,0 +1,12 @@
+; RUN: split-file %s %t
+; RUN: not llvm-as < %t/aarch64-svcount.ll -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-AARCH64-SVCOUNT %s
+; RUN: not llvm-as < %t/riscv-vector-tuple.ll -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-RISCV-VECTOR-TUPLE %s
+; Check target extension type properties are verified in the assembler.
+
+;--- aarch64-svcount.ll
+declare target("aarch64.svcount", i32) @aarch64_svcount()
+; CHECK-AARCH64-SVCOUNT: error: Target type should have no parameters
+
+;--- riscv-vector-tuple.ll
+declare target("riscv.vector.tuple", 99) @riscv_vector_tuple()
+; CHECK-RISCV-VECTOR-TUPLE: Target type should have one type parameter and one integer parameter
>From 31d987eaf0f7fdaabc63a3a1c659cf99de2b0805 Mon Sep 17 00:00:00 2001
From: Jay Foad <jay.foad at amd.com>
Date: Thu, 5 Sep 2024 11:42:01 +0100
Subject: [PATCH 2/6] Better error messages
---
llvm/lib/IR/Type.cpp | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index 0bb9b282385ac7..cfbdb36dbb435a 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -828,13 +828,14 @@ Expected<TargetExtType *> TargetExtType::checkParams(TargetExtType *TTy) {
// Opaque types in the AArch64 name space.
if (TTy->Name == "aarch64.svcount" &&
(TTy->getNumTypeParameters() != 0 || TTy->getNumIntParameters() != 0))
- return createStringError("Target type should have no parameters");
+ return createStringError(
+ "Target type aarch64.svcount should have no parameters");
// Opaque types in the RISC-V name space.
if (TTy->Name == "riscv.vector.tuple" &&
(TTy->getNumTypeParameters() != 1 || TTy->getNumIntParameters() != 1))
- return createStringError(
- "Target type should have one type parameter and one integer parameter");
+ return createStringError("Target type riscv.vector.tuple should have one "
+ "type parameter and one integer parameter");
return TTy;
}
>From f67b691b99cbe09878e6a083a830b60774c00207 Mon Sep 17 00:00:00 2001
From: Jay Foad <jay.foad at amd.com>
Date: Thu, 5 Sep 2024 11:44:28 +0100
Subject: [PATCH 3/6] Update tests
---
llvm/test/Assembler/target-type-param-errors.ll | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/llvm/test/Assembler/target-type-param-errors.ll b/llvm/test/Assembler/target-type-param-errors.ll
index a356a0804fafa7..acc22f4a712d7b 100644
--- a/llvm/test/Assembler/target-type-param-errors.ll
+++ b/llvm/test/Assembler/target-type-param-errors.ll
@@ -5,8 +5,8 @@
;--- aarch64-svcount.ll
declare target("aarch64.svcount", i32) @aarch64_svcount()
-; CHECK-AARCH64-SVCOUNT: error: Target type should have no parameters
+; CHECK-AARCH64-SVCOUNT: error: Target type aarch64.svcount should have no parameters
;--- riscv-vector-tuple.ll
declare target("riscv.vector.tuple", 99) @riscv_vector_tuple()
-; CHECK-RISCV-VECTOR-TUPLE: Target type should have one type parameter and one integer parameter
+; CHECK-RISCV-VECTOR-TUPLE: Target type riscv.vector.tuple should have one type parameter and one integer parameter
>From 4c43176652a215c046a468ef8a45a59acd4598c6 Mon Sep 17 00:00:00 2001
From: Jay Foad <jay.foad at amd.com>
Date: Thu, 5 Sep 2024 11:56:17 +0100
Subject: [PATCH 4/6] Add error checking in BitcodeReader
---
llvm/lib/Bitcode/Reader/BitcodeReader.cpp | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 654be985a3229c..12bf0936bac908 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -2679,7 +2679,11 @@ Error BitcodeReader::parseTypeTableBody() {
return error("Integer parameter too large");
IntParams.push_back(Record[i]);
}
- ResultTy = TargetExtType::get(Context, TypeName, TypeParams, IntParams);
+ auto TTy =
+ TargetExtType::getOrError(Context, TypeName, TypeParams, IntParams);
+ if (auto E = TTy.takeError())
+ return error(toString(std::move(E)));
+ ResultTy = *TTy;
TypeName.clear();
break;
}
>From 5da45f84e3192e9ec62db792041bf13fd45d9ee4 Mon Sep 17 00:00:00 2001
From: Jay Foad <jay.foad at amd.com>
Date: Thu, 5 Sep 2024 15:15:05 +0100
Subject: [PATCH 5/6] Simplify error handling in bitcode reader
---
llvm/lib/Bitcode/Reader/BitcodeReader.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 12bf0936bac908..1cd9ec6b8fca20 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -2682,7 +2682,7 @@ Error BitcodeReader::parseTypeTableBody() {
auto TTy =
TargetExtType::getOrError(Context, TypeName, TypeParams, IntParams);
if (auto E = TTy.takeError())
- return error(toString(std::move(E)));
+ return E;
ResultTy = *TTy;
TypeName.clear();
break;
>From c24aead8120a5514c2bb31d6e9ad28e69d28a7dd Mon Sep 17 00:00:00 2001
From: Jay Foad <jay.foad at amd.com>
Date: Thu, 5 Sep 2024 15:40:21 +0100
Subject: [PATCH 6/6] Tweak error messages and lower case
---
llvm/lib/IR/Type.cpp | 7 ++++---
llvm/test/Assembler/target-type-param-errors.ll | 4 ++--
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index cfbdb36dbb435a..93891461dd663f 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -829,13 +829,14 @@ Expected<TargetExtType *> TargetExtType::checkParams(TargetExtType *TTy) {
if (TTy->Name == "aarch64.svcount" &&
(TTy->getNumTypeParameters() != 0 || TTy->getNumIntParameters() != 0))
return createStringError(
- "Target type aarch64.svcount should have no parameters");
+ "target extension type aarch64.svcount should have no parameters");
// Opaque types in the RISC-V name space.
if (TTy->Name == "riscv.vector.tuple" &&
(TTy->getNumTypeParameters() != 1 || TTy->getNumIntParameters() != 1))
- return createStringError("Target type riscv.vector.tuple should have one "
- "type parameter and one integer parameter");
+ return createStringError(
+ "target extension type riscv.vector.tuple should have one "
+ "type parameter and one integer parameter");
return TTy;
}
diff --git a/llvm/test/Assembler/target-type-param-errors.ll b/llvm/test/Assembler/target-type-param-errors.ll
index acc22f4a712d7b..03180811c7549e 100644
--- a/llvm/test/Assembler/target-type-param-errors.ll
+++ b/llvm/test/Assembler/target-type-param-errors.ll
@@ -5,8 +5,8 @@
;--- aarch64-svcount.ll
declare target("aarch64.svcount", i32) @aarch64_svcount()
-; CHECK-AARCH64-SVCOUNT: error: Target type aarch64.svcount should have no parameters
+; CHECK-AARCH64-SVCOUNT: error: target extension type aarch64.svcount should have no parameters
;--- riscv-vector-tuple.ll
declare target("riscv.vector.tuple", 99) @riscv_vector_tuple()
-; CHECK-RISCV-VECTOR-TUPLE: Target type riscv.vector.tuple should have one type parameter and one integer parameter
+; CHECK-RISCV-VECTOR-TUPLE: target extension type riscv.vector.tuple should have one type parameter and one integer parameter
More information about the llvm-commits
mailing list