Skip to content

Commit

Permalink
[UseListOrder] Fix use list order for function operands
Browse files Browse the repository at this point in the history 
Functions can have a personality function, as well as prefix and
prologue data as additional operands. Unused operands are assigned
a dummy value of i1* null. This patch addresses multiple issues in
use-list order preservation for these:

 * Fix verify-uselistorder to also enumerate the dummy values.
   This means that now use-list order values of these values are
   shuffled even if there is no other mention of i1* null in the
   module. This results in failures of Assembler/call-arg-is-callee.ll,
   Assembler/opaque-ptr.ll and Bitcode/use-list-order2.ll.
 * The use-list order prediction in ValueEnumerator does not take
   into account the fact that a global may use a value more than
   once and leaves uses in the same global effectively unordered.
   We should be comparing the operand number here, as we do for
   the more general case.
 * While we enumerate all operands of a function together (which
   seems sensible to me), the bitcode reader would first resolve
   prefix data for all function, then prologue data for all
   functions, then personality functions for all functions. Change
   this to resolve all operands for a given function together
   instead.

Differential Revision: https://reviews.llvm.org/D109282
  • Loading branch information
@nikic
nikic committed Sep 7, 2021
1 parent 7f54009 commit f5832ea
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 66 deletions.
102 changes: 53 additions & 49 deletions llvm/lib/Bitcode/Reader/BitcodeReader.cpp
View file
Expand Up @@ -501,9 +501,14 @@ class BitcodeReader : public BitcodeReaderBase, public GVMaterializer {

std::vector<std::pair<GlobalVariable *, unsigned>> GlobalInits;
std::vector<std::pair<GlobalIndirectSymbol *, unsigned>> IndirectSymbolInits;
std::vector<std::pair<Function *, unsigned>> FunctionPrefixes;
std::vector<std::pair<Function *, unsigned>> FunctionPrologues;
std::vector<std::pair<Function *, unsigned>> FunctionPersonalityFns;

struct FunctionOperandInfo {
Function *F;
unsigned PersonalityFn;
unsigned Prefix;
unsigned Prologue;
};
std::vector<FunctionOperandInfo> FunctionOperands;

/// The set of attributes by index. Index zero in the file is for null, and
/// is thus not represented here. As such all indices are off by one.
Expand Down Expand Up @@ -2244,15 +2249,11 @@ Error BitcodeReader::resolveGlobalAndIndirectSymbolInits() {
std::vector<std::pair<GlobalVariable *, unsigned>> GlobalInitWorklist;
std::vector<std::pair<GlobalIndirectSymbol *, unsigned>>
IndirectSymbolInitWorklist;
std::vector<std::pair<Function *, unsigned>> FunctionPrefixWorklist;
std::vector<std::pair<Function *, unsigned>> FunctionPrologueWorklist;
std::vector<std::pair<Function *, unsigned>> FunctionPersonalityFnWorklist;
std::vector<FunctionOperandInfo> FunctionOperandWorklist;

GlobalInitWorklist.swap(GlobalInits);
IndirectSymbolInitWorklist.swap(IndirectSymbolInits);
FunctionPrefixWorklist.swap(FunctionPrefixes);
FunctionPrologueWorklist.swap(FunctionPrologues);
FunctionPersonalityFnWorklist.swap(FunctionPersonalityFns);
FunctionOperandWorklist.swap(FunctionOperands);

while (!GlobalInitWorklist.empty()) {
unsigned ValID = GlobalInitWorklist.back().second;
Expand Down Expand Up @@ -2284,43 +2285,41 @@ Error BitcodeReader::resolveGlobalAndIndirectSymbolInits() {
IndirectSymbolInitWorklist.pop_back();
}

while (!FunctionPrefixWorklist.empty()) {
unsigned ValID = FunctionPrefixWorklist.back().second;
if (ValID >= ValueList.size()) {
FunctionPrefixes.push_back(FunctionPrefixWorklist.back());
} else {
if (Constant *C = dyn_cast_or_null<Constant>(ValueList[ValID]))
FunctionPrefixWorklist.back().first->setPrefixData(C);
else
return error("Expected a constant");
while (!FunctionOperandWorklist.empty()) {
FunctionOperandInfo &Info = FunctionOperandWorklist.back();
if (Info.PersonalityFn) {
unsigned ValID = Info.PersonalityFn - 1;
if (ValID < ValueList.size()) {
if (Constant *C = dyn_cast_or_null<Constant>(ValueList[ValID]))
Info.F->setPersonalityFn(C);
else
return error("Expected a constant");
Info.PersonalityFn = 0;
}
}
FunctionPrefixWorklist.pop_back();
}

while (!FunctionPrologueWorklist.empty()) {
unsigned ValID = FunctionPrologueWorklist.back().second;
if (ValID >= ValueList.size()) {
FunctionPrologues.push_back(FunctionPrologueWorklist.back());
} else {
if (Constant *C = dyn_cast_or_null<Constant>(ValueList[ValID]))
FunctionPrologueWorklist.back().first->setPrologueData(C);
else
return error("Expected a constant");
if (Info.Prefix) {
unsigned ValID = Info.Prefix - 1;
if (ValID < ValueList.size()) {
if (Constant *C = dyn_cast_or_null<Constant>(ValueList[ValID]))
Info.F->setPrefixData(C);
else
return error("Expected a constant");
Info.Prefix = 0;
}
}
FunctionPrologueWorklist.pop_back();
}

while (!FunctionPersonalityFnWorklist.empty()) {
unsigned ValID = FunctionPersonalityFnWorklist.back().second;
if (ValID >= ValueList.size()) {
FunctionPersonalityFns.push_back(FunctionPersonalityFnWorklist.back());
} else {
if (Constant *C = dyn_cast_or_null<Constant>(ValueList[ValID]))
FunctionPersonalityFnWorklist.back().first->setPersonalityFn(C);
else
return error("Expected a constant");
if (Info.Prologue) {
unsigned ValID = Info.Prologue - 1;
if (ValID < ValueList.size()) {
if (Constant *C = dyn_cast_or_null<Constant>(ValueList[ValID]))
Info.F->setPrologueData(C);
else
return error("Expected a constant");
Info.Prologue = 0;
}
}
FunctionPersonalityFnWorklist.pop_back();
if (Info.PersonalityFn || Info.Prefix || Info.Prologue)
FunctionOperands.push_back(Info);
FunctionOperandWorklist.pop_back();
}

return Error::success();
Expand Down Expand Up @@ -3385,8 +3384,10 @@ Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {
if (Record.size() > 9)
UnnamedAddr = getDecodedUnnamedAddrType(Record[9]);
Func->setUnnamedAddr(UnnamedAddr);
if (Record.size() > 10 && Record[10] != 0)
FunctionPrologues.push_back(std::make_pair(Func, Record[10] - 1));

FunctionOperandInfo OperandInfo = {Func, 0, 0, 0};
if (Record.size() > 10)
OperandInfo.Prologue = Record[10];

if (Record.size() > 11)
Func->setDLLStorageClass(getDecodedDLLStorageClass(Record[11]));
Expand All @@ -3403,11 +3404,11 @@ Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {
Func->setComdat(reinterpret_cast<Comdat *>(1));
}

if (Record.size() > 13 && Record[13] != 0)
FunctionPrefixes.push_back(std::make_pair(Func, Record[13] - 1));
if (Record.size() > 13)
OperandInfo.Prefix = Record[13];

if (Record.size() > 14 && Record[14] != 0)
FunctionPersonalityFns.push_back(std::make_pair(Func, Record[14] - 1));
if (Record.size() > 14)
OperandInfo.PersonalityFn = Record[14];

if (Record.size() > 15) {
Func->setDSOLocal(getDecodedDSOLocal(Record[15]));
Expand All @@ -3425,6 +3426,9 @@ Error BitcodeReader::parseFunctionRecord(ArrayRef<uint64_t> Record) {

ValueList.push_back(Func);

if (OperandInfo.PersonalityFn || OperandInfo.Prefix || OperandInfo.Prologue)
FunctionOperands.push_back(OperandInfo);

// If this is a function with a body, remember the prototype we are
// creating now, so that we can match up the body with them later.
if (!isProto) {
Expand Down
5 changes: 4 additions & 1 deletion llvm/lib/Bitcode/Writer/ValueEnumerator.cpp
View file
Expand Up @@ -229,8 +229,11 @@ static void predictValueUseListOrderImpl(const Value *V, const Function *F,
// have been read (despite having earlier IDs). Rather than awkwardly
// modeling this behaviour here, orderModule() has assigned IDs to
// initializers of GlobalValues before GlobalValues themselves.
if (OM.isGlobalValue(LID) && OM.isGlobalValue(RID))
if (OM.isGlobalValue(LID) && OM.isGlobalValue(RID)) {
if (LID == RID)
return LU->getOperandNo() > RU->getOperandNo();
return LID < RID;
}

// If ID is 4, then expect: 7 6 5 1 2 3.
if (LID < RID) {
Expand Down
11 changes: 11 additions & 0 deletions llvm/test/Assembler/function-operand-uselistorder.ll
View file
@@ -0,0 +1,11 @@
; RUN: verify-uselistorder %s

@g = global i8 0

define void @f1() prefix i8* @g prologue i8* @g personality i8* @g {
ret void
}

define void @f2() prefix i8* @g prologue i8* @g personality i8* @g {
ret void
}
22 changes: 6 additions & 16 deletions llvm/tools/verify-uselistorder/verify-uselistorder.cpp
View file
Expand Up @@ -202,14 +202,9 @@ ValueMapping::ValueMapping(const Module &M) {
map(A.getAliasee());
for (const GlobalIFunc &IF : M.ifuncs())
map(IF.getResolver());
for (const Function &F : M) {
if (F.hasPrefixData())
map(F.getPrefixData());
if (F.hasPrologueData())
map(F.getPrologueData());
if (F.hasPersonalityFn())
map(F.getPersonalityFn());
}
for (const Function &F : M)
for (Value *Op : F.operands())
map(Op);

// Function bodies.
for (const Function &F : M) {
Expand Down Expand Up @@ -484,14 +479,9 @@ static void changeUseLists(Module &M, Changer changeValueUseList) {
changeValueUseList(A.getAliasee());
for (GlobalIFunc &IF : M.ifuncs())
changeValueUseList(IF.getResolver());
for (Function &F : M) {
if (F.hasPrefixData())
changeValueUseList(F.getPrefixData());
if (F.hasPrologueData())
changeValueUseList(F.getPrologueData());
if (F.hasPersonalityFn())
changeValueUseList(F.getPersonalityFn());
}
for (Function &F : M)
for (Value *Op : F.operands())
changeValueUseList(Op);

// Function bodies.
for (Function &F : M) {
Expand Down

0 comments on commit f5832ea

Please sign in to comment.