[llvm] [AsmParser] Don't require value numbers to be consecutive (PR #78171)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 15 07:38:38 PST 2024
https://github.com/nikic created https://github.com/llvm/llvm-project/pull/78171
Currently, the IR parser requires that %n style numbered values are consecutive. This means that the IR becomes invalid as soon as you remove an instruction, argument or block. This makes it very annoying to modify IR without running it through instnamer first.
I don't think there is any good reason to impose this requirement. This PR relaxes it to allow value IDs to be non-consecutive, but it still keeps the requirement that they're increasing (i.e. you can't skip a value number and then assign it later).
I've kept the underlying representation the same and added a guard against skipping large ranges to guard against degenerate cases where the NumberedVals vector would become huge. If people prefer, I could change NumberedVals from a vector to a map + a counter instead, in which case we could support arbitrary numbers without large increases in memory usage.
This only implements support for skipping numbers for local values. We should extend this to global values in the future as well.
>From 15e440725c35b474a871b0da33d7992d917977e4 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Mon, 15 Jan 2024 15:50:44 +0100
Subject: [PATCH] [AsmParser] Don't require value numbers to be consecutive
Currently, the IR parser requires that %n style numbered values
are consecutive. This means that the IR becomes invalid as soon
as you remove an instruction, argument or block. This makes it
very annoying to modify IR without running it through instnamer
first.
I don't think there is any good reason to impose this requirement.
This PR relaxes it to allow value IDs to be non-consecutive, but
it still keeps the requirement that they're increasing (i.e. you
can't skip a value number and then assign it later).
I've kept the underlying representation the same and added a guard
against skipping large ranges to guard against degenerate cases
where the NumberedVals vector would become huge. If people prefer,
I could change NumberedVals from a vector to a map + a counter
instead, in which case we could support arbitrary numbers without
large increases in memory usage.
This only implements support for skipping numbers for local values.
We should extend this to global values in the future as well.
---
llvm/include/llvm/AsmParser/LLParser.h | 16 ++-
llvm/lib/AsmParser/LLParser.cpp | 105 +++++++++++++-----
.../call-nonzero-program-addrspace-2.ll | 2 +-
llvm/test/Assembler/invalid-arg-num-1.ll | 6 -
llvm/test/Assembler/invalid-arg-num-2.ll | 6 -
llvm/test/Assembler/invalid-arg-num-3.ll | 6 -
.../test/Assembler/invalid-block-label-num.ll | 7 --
.../Assembler/skip-value-numbers-invalid.ll | 61 ++++++++++
llvm/test/Assembler/skip-value-numbers.ll | 79 +++++++++++++
9 files changed, 228 insertions(+), 60 deletions(-)
delete mode 100644 llvm/test/Assembler/invalid-arg-num-1.ll
delete mode 100644 llvm/test/Assembler/invalid-arg-num-2.ll
delete mode 100644 llvm/test/Assembler/invalid-arg-num-3.ll
delete mode 100644 llvm/test/Assembler/invalid-block-label-num.ll
create mode 100644 llvm/test/Assembler/skip-value-numbers-invalid.ll
create mode 100644 llvm/test/Assembler/skip-value-numbers.ll
diff --git a/llvm/include/llvm/AsmParser/LLParser.h b/llvm/include/llvm/AsmParser/LLParser.h
index 54bc3e582e01ae..bfc17860b9525f 100644
--- a/llvm/include/llvm/AsmParser/LLParser.h
+++ b/llvm/include/llvm/AsmParser/LLParser.h
@@ -203,6 +203,9 @@ namespace llvm {
bool error(LocTy L, const Twine &Msg) const { return Lex.Error(L, Msg); }
bool tokError(const Twine &Msg) const { return error(Lex.getLoc(), Msg); }
+ bool checkValueID(LocTy L, StringRef Kind, StringRef Prefix,
+ unsigned NextID, unsigned ID) const;
+
/// Restore the internal name and slot mappings using the mappings that
/// were created at an earlier parsing stage.
void restoreParsingState(const SlotMapping *Slots);
@@ -459,8 +462,10 @@ namespace llvm {
/// FunctionNumber - If this is an unnamed function, this is the slot
/// number of it, otherwise it is -1.
int FunctionNumber;
+
public:
- PerFunctionState(LLParser &p, Function &f, int functionNumber);
+ PerFunctionState(LLParser &p, Function &f, int functionNumber,
+ ArrayRef<unsigned> UnnamedArgNums);
~PerFunctionState();
Function &getFunction() const { return F; }
@@ -590,9 +595,12 @@ namespace llvm {
ArgInfo(LocTy L, Type *ty, AttributeSet Attr, const std::string &N)
: Loc(L), Ty(ty), Attrs(Attr), Name(N) {}
};
- bool parseArgumentList(SmallVectorImpl<ArgInfo> &ArgList, bool &IsVarArg);
- bool parseFunctionHeader(Function *&Fn, bool IsDefine);
- bool parseFunctionBody(Function &Fn);
+ bool parseArgumentList(SmallVectorImpl<ArgInfo> &ArgList,
+ SmallVectorImpl<unsigned> &UnnamedArgNums,
+ bool &IsVarArg);
+ bool parseFunctionHeader(Function *&Fn, bool IsDefine,
+ SmallVectorImpl<unsigned> &UnnamedArgNums);
+ bool parseFunctionBody(Function &Fn, ArrayRef<unsigned> UnnamedArgNums);
bool parseBasicBlock(PerFunctionState &PFS);
enum TailCallType { TCT_None, TCT_Tail, TCT_MustTail };
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index fb9e1ba875e1fa..20e5ae2c88764a 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -54,6 +54,9 @@
using namespace llvm;
+// Avoid large gaps in the NumberedVals vector.
+static const size_t MaxNumberedValSkip = 1000;
+
static std::string getTypeString(Type *T) {
std::string Result;
raw_string_ostream Tmp(Result);
@@ -572,7 +575,8 @@ bool LLParser::parseDeclare() {
}
Function *F;
- if (parseFunctionHeader(F, false))
+ SmallVector<unsigned> UnnamedArgNums;
+ if (parseFunctionHeader(F, false, UnnamedArgNums))
return true;
for (auto &MD : MDs)
F->addMetadata(MD.first, *MD.second);
@@ -586,8 +590,10 @@ bool LLParser::parseDefine() {
Lex.Lex();
Function *F;
- return parseFunctionHeader(F, true) || parseOptionalFunctionMetadata(*F) ||
- parseFunctionBody(*F);
+ SmallVector<unsigned> UnnamedArgNums;
+ return parseFunctionHeader(F, true, UnnamedArgNums) ||
+ parseOptionalFunctionMetadata(*F) ||
+ parseFunctionBody(*F, UnnamedArgNums);
}
/// parseGlobalType
@@ -2925,6 +2931,19 @@ bool LLParser::parseOptionalOperandBundles(
return false;
}
+bool LLParser::checkValueID(LocTy Loc, StringRef Kind, StringRef Prefix,
+ unsigned NextID, unsigned ID) const {
+ if (ID < NextID)
+ return error(Loc, Kind + " expected to be numbered '" + Prefix +
+ Twine(NextID) + "' or greater");
+
+ if (ID > NextID + MaxNumberedValSkip)
+ return error(Loc, "value numbers can skip at most " +
+ Twine(MaxNumberedValSkip) + " values");
+
+ return false;
+}
+
/// parseArgumentList - parse the argument list for a function type or function
/// prototype.
/// ::= '(' ArgTypeListI ')'
@@ -2935,6 +2954,7 @@ bool LLParser::parseOptionalOperandBundles(
/// ::= ArgType (',' ArgType)*
///
bool LLParser::parseArgumentList(SmallVectorImpl<ArgInfo> &ArgList,
+ SmallVectorImpl<unsigned> &UnnamedArgNums,
bool &IsVarArg) {
unsigned CurValID = 0;
IsVarArg = false;
@@ -2961,12 +2981,19 @@ bool LLParser::parseArgumentList(SmallVectorImpl<ArgInfo> &ArgList,
if (Lex.getKind() == lltok::LocalVar) {
Name = Lex.getStrVal();
Lex.Lex();
- } else if (Lex.getKind() == lltok::LocalVarID) {
- if (Lex.getUIntVal() != CurValID)
- return error(TypeLoc, "argument expected to be numbered '%" +
- Twine(CurValID) + "'");
- ++CurValID;
- Lex.Lex();
+ } else {
+ unsigned ArgID;
+ if (Lex.getKind() == lltok::LocalVarID) {
+ ArgID = Lex.getUIntVal();
+ if (checkValueID(TypeLoc, "argument", "%", CurValID, ArgID))
+ return true;
+ Lex.Lex();
+ } else {
+ ArgID = CurValID;
+ }
+
+ UnnamedArgNums.push_back(ArgID);
+ CurValID = ArgID + 1;
}
if (!FunctionType::isValidArgumentType(ArgTy))
@@ -2995,13 +3022,17 @@ bool LLParser::parseArgumentList(SmallVectorImpl<ArgInfo> &ArgList,
Name = Lex.getStrVal();
Lex.Lex();
} else {
+ unsigned ArgID;
if (Lex.getKind() == lltok::LocalVarID) {
- if (Lex.getUIntVal() != CurValID)
- return error(TypeLoc, "argument expected to be numbered '%" +
- Twine(CurValID) + "'");
+ ArgID = Lex.getUIntVal();
+ if (checkValueID(TypeLoc, "argument", "%", CurValID, ArgID))
+ return true;
Lex.Lex();
+ } else {
+ ArgID = CurValID;
}
- ++CurValID;
+ UnnamedArgNums.push_back(ArgID);
+ CurValID = ArgID + 1;
Name = "";
}
@@ -3027,7 +3058,8 @@ bool LLParser::parseFunctionType(Type *&Result) {
SmallVector<ArgInfo, 8> ArgList;
bool IsVarArg;
- if (parseArgumentList(ArgList, IsVarArg))
+ SmallVector<unsigned> UnnamedArgNums;
+ if (parseArgumentList(ArgList, UnnamedArgNums, IsVarArg))
return true;
// Reject names on the arguments lists.
@@ -3263,13 +3295,19 @@ bool LLParser::parseTargetExtType(Type *&Result) {
//===----------------------------------------------------------------------===//
LLParser::PerFunctionState::PerFunctionState(LLParser &p, Function &f,
- int functionNumber)
+ int functionNumber,
+ ArrayRef<unsigned> UnnamedArgNums)
: P(p), F(f), FunctionNumber(functionNumber) {
// Insert unnamed arguments into the NumberedVals list.
- for (Argument &A : F.args())
- if (!A.hasName())
+ auto It = UnnamedArgNums.begin();
+ for (Argument &A : F.args()) {
+ if (!A.hasName()) {
+ unsigned ArgNum = *It++;
+ NumberedVals.resize(ArgNum);
NumberedVals.push_back(&A);
+ }
+ }
}
LLParser::PerFunctionState::~PerFunctionState() {
@@ -3400,9 +3438,9 @@ bool LLParser::PerFunctionState::setInstName(int NameID,
if (NameID == -1)
NameID = NumberedVals.size();
- if (unsigned(NameID) != NumberedVals.size())
- return P.error(NameLoc, "instruction expected to be numbered '%" +
- Twine(NumberedVals.size()) + "'");
+ if (P.checkValueID(NameLoc, "instruction", "%", NumberedVals.size(),
+ NameID))
+ return true;
auto FI = ForwardRefValIDs.find(NameID);
if (FI != ForwardRefValIDs.end()) {
@@ -3417,6 +3455,9 @@ bool LLParser::PerFunctionState::setInstName(int NameID,
ForwardRefValIDs.erase(FI);
}
+ // Fill in skipped IDs using nullptr.
+ NumberedVals.resize(unsigned(NameID));
+
NumberedVals.push_back(Inst);
return false;
}
@@ -3464,12 +3505,13 @@ BasicBlock *LLParser::PerFunctionState::defineBB(const std::string &Name,
int NameID, LocTy Loc) {
BasicBlock *BB;
if (Name.empty()) {
- if (NameID != -1 && unsigned(NameID) != NumberedVals.size()) {
- P.error(Loc, "label expected to be numbered '" +
- Twine(NumberedVals.size()) + "'");
- return nullptr;
+ if (NameID != -1) {
+ if (P.checkValueID(Loc, "label", "", NumberedVals.size(), NameID))
+ return nullptr;
+ } else {
+ NameID = NumberedVals.size();
}
- BB = getBB(NumberedVals.size(), Loc);
+ BB = getBB(NameID, Loc);
if (!BB) {
P.error(Loc, "unable to create block numbered '" +
Twine(NumberedVals.size()) + "'");
@@ -3489,7 +3531,8 @@ BasicBlock *LLParser::PerFunctionState::defineBB(const std::string &Name,
// Remove the block from forward ref sets.
if (Name.empty()) {
- ForwardRefValIDs.erase(NumberedVals.size());
+ ForwardRefValIDs.erase(NameID);
+ NumberedVals.resize(NameID);
NumberedVals.push_back(BB);
} else {
// BB forward references are already in the function symbol table.
@@ -5934,7 +5977,8 @@ bool LLParser::parseTypeAndBasicBlock(BasicBlock *&BB, LocTy &Loc,
/// OptionalCallingConv OptRetAttrs OptUnnamedAddr Type GlobalName
/// '(' ArgList ')' OptAddrSpace OptFuncAttrs OptSection OptionalAlign
/// OptGC OptionalPrefix OptionalPrologue OptPersonalityFn
-bool LLParser::parseFunctionHeader(Function *&Fn, bool IsDefine) {
+bool LLParser::parseFunctionHeader(Function *&Fn, bool IsDefine,
+ SmallVectorImpl<unsigned> &UnnamedArgNums) {
// parse the linkage.
LocTy LinkageLoc = Lex.getLoc();
unsigned Linkage;
@@ -6022,7 +6066,7 @@ bool LLParser::parseFunctionHeader(Function *&Fn, bool IsDefine) {
Constant *PersonalityFn = nullptr;
Comdat *C;
- if (parseArgumentList(ArgList, IsVarArg) ||
+ if (parseArgumentList(ArgList, UnnamedArgNums, IsVarArg) ||
parseOptionalUnnamedAddr(UnnamedAddr) ||
parseOptionalProgramAddrSpace(AddrSpace) ||
parseFnAttributeValuePairs(FuncAttrs, FwdRefAttrGrps, false,
@@ -6217,7 +6261,8 @@ bool LLParser::PerFunctionState::resolveForwardRefBlockAddresses() {
/// parseFunctionBody
/// ::= '{' BasicBlock+ UseListOrderDirective* '}'
-bool LLParser::parseFunctionBody(Function &Fn) {
+bool LLParser::parseFunctionBody(Function &Fn,
+ ArrayRef<unsigned> UnnamedArgNums) {
if (Lex.getKind() != lltok::lbrace)
return tokError("expected '{' in function body");
Lex.Lex(); // eat the {.
@@ -6225,7 +6270,7 @@ bool LLParser::parseFunctionBody(Function &Fn) {
int FunctionNumber = -1;
if (!Fn.hasName()) FunctionNumber = NumberedVals.size()-1;
- PerFunctionState PFS(*this, Fn, FunctionNumber);
+ PerFunctionState PFS(*this, Fn, FunctionNumber, UnnamedArgNums);
// Resolve block addresses and allow basic blocks to be forward-declared
// within this function.
diff --git a/llvm/test/Assembler/call-nonzero-program-addrspace-2.ll b/llvm/test/Assembler/call-nonzero-program-addrspace-2.ll
index f5c5437d8221e6..bc600d56db51b2 100644
--- a/llvm/test/Assembler/call-nonzero-program-addrspace-2.ll
+++ b/llvm/test/Assembler/call-nonzero-program-addrspace-2.ll
@@ -3,7 +3,7 @@
; Check that numbered variables in a nonzero program address space 200 can be used in a call instruction
-define i8 @test_unnamed(ptr, ptr addrspace(42) %0) {
+define i8 @test_unnamed(ptr, ptr addrspace(42) %1) {
; Calls with explicit address spaces are fine:
call addrspace(0) i8 %0(i32 0)
call addrspace(42) i8 %1(i32 0)
diff --git a/llvm/test/Assembler/invalid-arg-num-1.ll b/llvm/test/Assembler/invalid-arg-num-1.ll
deleted file mode 100644
index ee13c339dec448..00000000000000
--- a/llvm/test/Assembler/invalid-arg-num-1.ll
+++ /dev/null
@@ -1,6 +0,0 @@
-; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
-
-; CHECK: error: argument expected to be numbered '%1'
-define void @foo(i32 %0, i32 %5) {
- ret void
-}
diff --git a/llvm/test/Assembler/invalid-arg-num-2.ll b/llvm/test/Assembler/invalid-arg-num-2.ll
deleted file mode 100644
index 6ecb0033512d00..00000000000000
--- a/llvm/test/Assembler/invalid-arg-num-2.ll
+++ /dev/null
@@ -1,6 +0,0 @@
-; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
-
-; CHECK: error: argument expected to be numbered '%1'
-define void @foo(i8 %0, i32 %named, i32 %2) {
- ret void
-}
diff --git a/llvm/test/Assembler/invalid-arg-num-3.ll b/llvm/test/Assembler/invalid-arg-num-3.ll
deleted file mode 100644
index f1638365630be5..00000000000000
--- a/llvm/test/Assembler/invalid-arg-num-3.ll
+++ /dev/null
@@ -1,6 +0,0 @@
-; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
-
-; CHECK: error: argument expected to be numbered '%0'
-define void @foo(i8 %1) {
- ret void
-}
diff --git a/llvm/test/Assembler/invalid-block-label-num.ll b/llvm/test/Assembler/invalid-block-label-num.ll
deleted file mode 100644
index 4f02300849e68f..00000000000000
--- a/llvm/test/Assembler/invalid-block-label-num.ll
+++ /dev/null
@@ -1,7 +0,0 @@
-; RUN: not llvm-as < %s 2>&1 | FileCheck %s
-
-define void @f () {
-1:
-; CHECK: error: label expected to be numbered '0'
- ret void
-}
diff --git a/llvm/test/Assembler/skip-value-numbers-invalid.ll b/llvm/test/Assembler/skip-value-numbers-invalid.ll
new file mode 100644
index 00000000000000..977110c22b4332
--- /dev/null
+++ b/llvm/test/Assembler/skip-value-numbers-invalid.ll
@@ -0,0 +1,61 @@
+; RUN: split-file %s %t
+; RUN: not llvm-as < %s %t/instr_smaller_id.ll 2>&1 | FileCheck %s --check-prefix=INSTR-SMALLER-ID
+; RUN: not llvm-as < %s %t/instr_too_large_skip.ll 2>&1 | FileCheck %s --check-prefix=INSTR-TOO-LARGE-SKIP
+; RUN: not llvm-as < %s %t/arg_smaller_id.ll 2>&1 | FileCheck %s --check-prefix=ARG-SMALLER-ID
+; RUN: not llvm-as < %s %t/arg_too_large_skip.ll 2>&1 | FileCheck %s --check-prefix=ARG-TOO-LARGE-SKIP
+; RUN: not llvm-as < %s %t/block_smaller_id.ll 2>&1 | FileCheck %s --check-prefix=BLOCK-SMALLER-ID
+; RUN: not llvm-as < %s %t/block_too_large_skip.ll 2>&1 | FileCheck %s --check-prefix=BLOCK-TOO-LARGE-SKIP
+
+;--- instr_smaller_id.ll
+
+; INSTR-SMALLER-ID: error: instruction expected to be numbered '%11' or greater
+define i32 @test() {
+ %10 = add i32 1, 2
+ %5 = add i32 %10, 3
+ ret i32 %5
+}
+
+;--- instr_too_large_skip.ll
+
+; INSTR-TOO-LARGE-SKIP: error: value numbers can skip at most 1000 values
+define i32 @test() {
+ %10 = add i32 1, 2
+ %1000000 = add i32 %10, 3
+ ret i32 %1000000
+}
+
+;--- arg_smaller_id.ll
+
+; ARG-SMALLER-ID: error: argument expected to be numbered '%11' or greater
+define i32 @test(i32 %10, i32 %5) {
+ ret i32 %5
+}
+
+;--- arg_too_large_skip.ll
+
+; ARG-TOO-LARGE-SKIP: error: value numbers can skip at most 1000 values
+define i32 @test(i32 %10, i32 %1000000) {
+ ret i32 %1000000
+}
+
+;--- block_smaller_id.ll
+
+; BLOCK-SMALLER-ID: error: label expected to be numbered '11' or greater
+define i32 @test() {
+10:
+ br label %5
+
+5:
+ ret i32 0
+}
+
+;--- block_too_large_skip.ll
+
+; BLOCK-TOO-LARGE-SKIP: error: value numbers can skip at most 1000 values
+define i32 @test() {
+10:
+ br label %1000000
+
+1000000:
+ ret i32 0
+}
diff --git a/llvm/test/Assembler/skip-value-numbers.ll b/llvm/test/Assembler/skip-value-numbers.ll
new file mode 100644
index 00000000000000..d74d8d502709b9
--- /dev/null
+++ b/llvm/test/Assembler/skip-value-numbers.ll
@@ -0,0 +1,79 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: -p --version 4
+; RUN: opt -S < %s | FileCheck %s
+
+define i32 @instr() {
+; CHECK-LABEL: define i32 @instr() {
+; CHECK-NEXT: %1 = add i32 1, 2
+; CHECK-NEXT: %2 = add i32 %1, 3
+; CHECK-NEXT: %3 = add i32 %2, 4
+; CHECK-NEXT: ret i32 %3
+;
+ %10 = add i32 1, 2
+ %20 = add i32 %10, 3
+ %30 = add i32 %20, 4
+ ret i32 %30
+}
+
+define i32 @instr_some_implicit() {
+; CHECK-LABEL: define i32 @instr_some_implicit() {
+; CHECK-NEXT: %1 = add i32 1, 2
+; CHECK-NEXT: %2 = add i32 3, 4
+; CHECK-NEXT: %3 = add i32 %1, %2
+; CHECK-NEXT: ret i32 %3
+;
+ add i32 1, 2
+ %10 = add i32 3, 4
+ add i32 %1, %10
+ ret i32 %11
+}
+
+define i32 @args(i32 %0, i32 %10, i32 %20) {
+; CHECK-LABEL: define i32 @args(i32 %0, i32 %1, i32 %2) {
+; CHECK-NEXT: %4 = add i32 %0, %1
+; CHECK-NEXT: %5 = add i32 %4, %2
+; CHECK-NEXT: ret i32 %5
+;
+ %30 = add i32 %0, %10
+ %31 = add i32 %30, %20
+ ret i32 %31
+}
+
+define i32 @args_some_implicit(i32, i32 %10, i32) {
+; CHECK-LABEL: define i32 @args_some_implicit(i32 %0, i32 %1, i32 %2) {
+; CHECK-NEXT: %4 = add i32 %0, %1
+; CHECK-NEXT: %5 = add i32 %2, %4
+; CHECK-NEXT: ret i32 %5
+;
+ add i32 %0, %10
+ %20 = add i32 %11, %13
+ ret i32 %20
+}
+
+define i32 @blocks() {
+; CHECK-LABEL: define i32 @blocks() {
+; CHECK-NEXT: br label %1
+; CHECK: 1:
+; CHECK-NEXT: ret i32 0
+;
+10:
+ br label %20
+
+20:
+ ret i32 0
+}
+
+define i32 @blocks_some_implicit() {
+; CHECK-LABEL: define i32 @blocks_some_implicit() {
+; CHECK-NEXT: br label %1
+; CHECK: 1:
+; CHECK-NEXT: br label %2
+; CHECK: 2:
+; CHECK-NEXT: ret i32 0
+;
+ br label %10
+
+10:
+ br label %11
+
+ ret i32 0
+}
More information about the llvm-commits
mailing list