Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC][AsmPrinter] Refactor FrameIndexExprs as a std::set #66433

Merged
merged 1 commit into from
Sep 20, 2023
Merged

[NFC][AsmPrinter] Refactor FrameIndexExprs as a std::set #66433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 17 additions & 30 deletions llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,20 @@ static DbgValueLoc getDebugLocValue(const MachineInstr *MI) {
return DbgValueLoc(Expr, DbgValueLocEntries, IsVariadic);
}

static uint64_t getFragmentOffsetInBits(const DIExpression &Expr) {
std::optional<DIExpression::FragmentInfo> Fragment = Expr.getFragmentInfo();
return Fragment ? Fragment->OffsetInBits : 0;
}

bool llvm::operator<(const FrameIndexExpr &LHS, const FrameIndexExpr &RHS) {
return getFragmentOffsetInBits(*LHS.Expr) <
getFragmentOffsetInBits(*RHS.Expr);
}

bool llvm::operator<(const EntryValueInfo &LHS, const EntryValueInfo &RHS) {
return getFragmentOffsetInBits(LHS.Expr) < getFragmentOffsetInBits(RHS.Expr);
}

Loc::Single::Single(DbgValueLoc ValueLoc)
: ValueLoc(std::make_unique<DbgValueLoc>(ValueLoc)),
Expr(ValueLoc.getExpression()) {
Expand All @@ -275,42 +289,15 @@ Loc::Single::Single(DbgValueLoc ValueLoc)
Loc::Single::Single(const MachineInstr *DbgValue)
: Single(getDebugLocValue(DbgValue)) {}

ArrayRef<FrameIndexExpr> Loc::MMI::getFrameIndexExprs() const {
if (FrameIndexExprs.size() == 1)
return FrameIndexExprs;

assert(llvm::all_of(
FrameIndexExprs,
[](const FrameIndexExpr &A) { return A.Expr->isFragment(); }) &&
"multiple FI expressions without DW_OP_LLVM_fragment");
llvm::sort(FrameIndexExprs,
[](const FrameIndexExpr &A, const FrameIndexExpr &B) -> bool {
return A.Expr->getFragmentInfo()->OffsetInBits <
B.Expr->getFragmentInfo()->OffsetInBits;
});

const std::set<FrameIndexExpr> &Loc::MMI::getFrameIndexExprs() const {
return FrameIndexExprs;
}

void Loc::MMI::addFrameIndexExpr(const DIExpression *Expr, int FI) {
// FIXME: This logic should not be necessary anymore, as we now have proper
// deduplication. However, without it, we currently run into the assertion
// below, which means that we are likely dealing with broken input, i.e. two
// non-fragment entries for the same variable at different frame indices.
if (FrameIndexExprs.size()) {
auto *Expr = FrameIndexExprs.back().Expr;
if (!Expr || !Expr->isFragment())
return;
}

if (llvm::none_of(FrameIndexExprs, [&](const FrameIndexExpr &Other) {
return FI == Other.FI && Expr == Other.Expr;
}))
FrameIndexExprs.push_back({FI, Expr});

FrameIndexExprs.insert({FI, Expr});
assert((FrameIndexExprs.size() == 1 ||
llvm::all_of(FrameIndexExprs,
[](FrameIndexExpr &FIE) {
[](const FrameIndexExpr &FIE) {
return FIE.Expr && FIE.Expr->isFragment();
})) &&
"conflicting locations for variable");
Expand Down
17 changes: 6 additions & 11 deletions llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ class DbgVariable;
struct FrameIndexExpr {
int FI;
const DIExpression *Expr;

/// Operator enabling sorting based on fragment offset.
friend bool operator<(const FrameIndexExpr &LHS, const FrameIndexExpr &RHS);
};

/// Represents an entry-value location, or a fragment of one.
Expand All @@ -115,15 +118,7 @@ struct EntryValueInfo {
const DIExpression &Expr;

/// Operator enabling sorting based on fragment offset.
bool operator<(const EntryValueInfo &Other) const {
return getFragmentOffsetInBits() < Other.getFragmentOffsetInBits();
}

private:
uint64_t getFragmentOffsetInBits() const {
std::optional<DIExpression::FragmentInfo> Fragment = Expr.getFragmentInfo();
return Fragment ? Fragment->OffsetInBits : 0;
}
friend bool operator<(const EntryValueInfo &LHS, const EntryValueInfo &RHS);
};

// Namespace for alternatives of a DbgVariable.
Expand Down Expand Up @@ -158,7 +153,7 @@ class Multi {
};
/// Single location defined by (potentially multiple) MMI entries.
struct MMI {
mutable SmallVector<FrameIndexExpr, 1> FrameIndexExprs;
std::set<FrameIndexExpr> FrameIndexExprs;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibilty would be a SetVector? (though then it's insertion ordered, rather than < ordered - not sure if that's sufficiently stable for the needs? perhaps not)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need the extra functionality provided by the SetVector here: our sorting keys (fragment offsets) are stable without the need for insertion-order-tracking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC the DWARF emitter also wants the fragments ordered by offset? I'm not sure what would happen if we were to emit DWARF fragment expressions out of order 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC the DWARF emitter also wants the fragments ordered by offset? I'm not sure what would happen if we were to emit DWARF fragment expressions out of order 🤔

Ah, yeah - DWARF does require the fragments be in order (essentially they don't say where they are in the structure - so you have to emit the first byte first, second byte second, etc)


public:
explicit MMI(const DIExpression *E, int FI) : FrameIndexExprs({{FI, E}}) {
Expand All @@ -167,7 +162,7 @@ struct MMI {
}
void addFrameIndexExpr(const DIExpression *Expr, int FI);
/// Get the FI entries, sorted by fragment offset.
ArrayRef<FrameIndexExpr> getFrameIndexExprs() const;
const std::set<FrameIndexExpr> &getFrameIndexExprs() const;
};
/// Single location defined by (potentially multiple) EntryValueInfo.
struct EntryValue {
Expand Down