Skip to content

[LLVM[NFC] Refactor to allow debug_names entries to conatain DIE offset #69399

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

Merged
merged 4 commits into from
Oct 25, 2023
Merged

[LLVM[NFC] Refactor to allow debug_names entries to conatain DIE offset #69399

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
103d7fd
[LLVM[NFC] Refactor to allow debug_names entries to conatain DIE offset
ayermolo Oct 17, 2023
06ae74e
fixup! [LLVM[NFC] Refactor to allow debug_names entries to conatain D…
ayermolo Oct 20, 2023
b3b4cb1
fixup! fixup! [LLVM[NFC] Refactor to allow debug_names entries to con…
ayermolo Oct 21, 2023
b1c90e8
fixup! fixup! fixup! [LLVM[NFC] Refactor to allow debug_names entries…
ayermolo Oct 23, 2023
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
21 changes: 15 additions & 6 deletions llvm/include/llvm/CodeGen/AccelTable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,20 +252,27 @@ class DWARF5AccelTableData : public AccelTableData {
public:
static uint32_t hash(StringRef Name) { return caseFoldingDjbHash(Name); }

DWARF5AccelTableData(const DIE &Die) : Die(Die) {}
DWARF5AccelTableData(const DIE &Die, const DwarfCompileUnit &CU);

#ifndef NDEBUG
void print(raw_ostream &OS) const override;
#endif

const DIE &getDie() const { return Die; }
uint64_t getDieOffset() const { return Die.getOffset(); }
unsigned getDieTag() const { return Die.getTag(); }
uint64_t getDieOffset() const {
if (const DIE *const *TDie = std::get_if<const DIE *>(&OffsetVal))
return (*TDie)->getOffset();
return std::get<uint64_t>(OffsetVal);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be better to make this assert-fail if it's stil in DIE mode, rather than offset mode? (to make it clear that that transformation is intended - I mean, if someone queries it before the DIEs have been laid out, DIE::getOffset will assert - but still, this seems like excess API surface area that could lead to more confusion later that'd be avoided by a hard stop here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about assert, but there are release builds that have asserts turned off.... So will result in "weird" crash.
Also getDieOffset is used in order() from ccelTableBase::finalize which is invoked before normalization happens.
One of the unit tests was failing because of it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about assert, but there are release builds that have asserts turned off.... So will result in "weird" crash.

Sure, though better than untested code being unidentifiable.

Also getDieOffset is used in order() from ccelTableBase::finalize which is invoked before normalization happens.
One of the unit tests was failing because of it.

Ah, OK - so at the moment this code is actually load bearing.

Could that be fixed/changed? Could normalizing happen before finalize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be moved to llvm::emitDWARF5AccelTable before Contents.finalize is invoked?
My original thought was to do normalization close to use, in context where it matters. In Dwarf5AccelTableWriter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be a somewhat simpler conceptual model if all uses of the DIE offsets in the accelerator tables came in the offset mode, after the DIE mode, if possible. Like after the DIE tree is finalized, we could finalize the accelerator in a similar way?

Like just after the computeSizeAndOffsets call in DwarfDebug::finalizeModuleInfo() we could do something similar to the accelerator tables? (call some function then take the sizes and offsets so computed, and bake them in/discard the DIE references)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let me take a look there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

something like fixup?
I went with path of least resistance of exposing Entries. Umm probably not ideal.
Alternative is for AccelTableBase to have some kind of member function that takes in a lambda to peform stuff on Entries. Although with all the templated code not sure how easy that will be.

Issue is that buckets are not available until AccelTableBase::finalize is called.

Overall I kind of feel weird calling some normalize/finalize thing after offsets are computed, that is just applicable to one thing.

}
unsigned getDieTag() const { return Tag; }
unsigned getUnitID() const { return UnitID; }
void normalizeDIEToOffset() { OffsetVal = getDieOffset(); }

protected:
const DIE &Die;
std::variant<const DIE *, uint64_t> OffsetVal;
dwarf::Tag Tag;
unsigned UnitID;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to end up increasing the size of the struct from 8 bytes to ((8(pointer/uint64)+8(discriminator and padding))+2(Tag)+2(padding)+4(uintID) == 24 bytes.
Might be worth skipping std::variant, so that the discriminator can be encoded in what's otherwise going to be 2 bytes of padding, then it'll only be 16 bytes (8(pointer/uint64)+2(discriminator)+2(tag)+4(uintID)) - and we could probably make uintIDs smaller, really - like they can probably be 16 bits just fine... (but that's a change for another time - it's fine here and wouldn't make the struct any smaller anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's a blocker I can redesign, but personally I think trying to squeeze most bits out just leads to a more fragile implementation. Plus cool c++ features. :D

Running llc on sema bitcode:
time ~/local/llvm-build-upstream-release/bin/llc -filetype=obj tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/Sema.cpp.ir.o -dwarf-version=5 -mtriple x86_64-pc-linux -o foo.o
with refactor

0:05.98 real, 5.80 user, 0.17 sys, 0 amem, 297044 mmem

0:06.18 real, 6.00 user, 0.17 sys, 0 amem, 296656 mmem

0:05.85 real, 5.69 user, 0.15 sys, 0 amem, 297264 mmem



main

0:05.52 real, 5.33 user, 0.17 sys, 0 amem, 297412 mmem

0:05.99 real, 5.82 user, 0.16 sys, 0 amem, 297068 mmem

0:05.87 real, 5.69 user, 0.16 sys, 0 amem, 297080 mmem

Seems like these changes fall under noise?


uint64_t order() const override { return Die.getOffset(); }
uint64_t order() const override { return getDieOffset(); }
};

class DWARF5AccelTableStaticData : public AccelTableData {
Expand All @@ -283,6 +290,8 @@ class DWARF5AccelTableStaticData : public AccelTableData {
uint64_t getDieOffset() const { return DieOffset; }
unsigned getDieTag() const { return DieTag; }
unsigned getCUIndex() const { return CUIndex; }
void normalizeDIEToOffset() { /* Not needed since DIE is not used. */
}

protected:
uint64_t DieOffset;
Expand Down
25 changes: 23 additions & 2 deletions llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,10 @@ class Dwarf5AccelTableWriter : public AccelTableWriter {
MCSymbol *AbbrevEnd = Asm->createTempSymbol("names_abbrev_end");
MCSymbol *EntryPool = Asm->createTempSymbol("names_entries");

/// Iterates over all the entries and if it contains a DIE replaces it with an
/// offset.
void normalizeOffsets();

DenseSet<uint32_t> getUniqueTags() const;

// Right now, we emit uniform attributes for all tags.
Expand Down Expand Up @@ -357,6 +361,13 @@ void AppleAccelTableWriter::emit() const {
emitData();
}

DWARF5AccelTableData::DWARF5AccelTableData(const DIE &Die,
const DwarfCompileUnit &CU)
: OffsetVal(&Die) {
Tag = Die.getTag();
UnitID = CU.getUniqueID();
}

template <typename DataT>
void Dwarf5AccelTableWriter<DataT>::Header::emit(Dwarf5AccelTableWriter &Ctx) {
assert(CompUnitCount > 0 && "Index must have at least one CU.");
Expand Down Expand Up @@ -387,6 +398,16 @@ void Dwarf5AccelTableWriter<DataT>::Header::emit(Dwarf5AccelTableWriter &Ctx) {
Asm->OutStreamer->emitBytes({AugmentationString, AugmentationStringSize});
}

template <typename DataT>
void Dwarf5AccelTableWriter<DataT>::normalizeOffsets() {
for (auto &Bucket : Contents.getBuckets()) {
for (auto *Hash : Bucket) {
for (auto *Value : Hash->Values) {
static_cast<DataT *>(Value)->normalizeDIEToOffset();
}
}
}
}
template <typename DataT>
DenseSet<uint32_t> Dwarf5AccelTableWriter<DataT>::getUniqueTags() const {
DenseSet<uint32_t> UniqueTags;
Expand Down Expand Up @@ -516,6 +537,7 @@ Dwarf5AccelTableWriter<DataT>::Dwarf5AccelTableWriter(
Header(CompUnits.size(), Contents.getBucketCount(),
Contents.getUniqueNameCount()),
CompUnits(CompUnits), getCUIndexForEntry(std::move(getCUIndexForEntry)) {
normalizeOffsets();
DenseSet<uint32_t> UniqueTags = getUniqueTags();
SmallVector<AttributeEncoding, 2> UniformAttributes = getUniformAttributes();

Expand Down Expand Up @@ -575,8 +597,7 @@ void llvm::emitDWARF5AccelTable(
Dwarf5AccelTableWriter<DWARF5AccelTableData>(
Asm, Contents, CompUnits,
[&](const DWARF5AccelTableData &Entry) {
const DIE *CUDie = Entry.getDie().getUnitDie();
return CUIndex[DD.lookupCU(CUDie)->getUniqueID()];
return CUIndex[Entry.getUnitID()];
})
.emit();
}
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3547,9 +3547,11 @@ void DwarfDebug::addAccelNameImpl(const DICompileUnit &CU,
case AccelTableKind::Apple:
AppleAccel.addName(Ref, Die);
break;
case AccelTableKind::Dwarf:
AccelDebugNames.addName(Ref, Die);
case AccelTableKind::Dwarf: {
DwarfCompileUnit *CU = lookupCU(Die.getUnitDie());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the addAccelNameImpl's CU parameter not accurate/correct here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well input there is DICompileUnit, which doesn't have uniqueID set yet, I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I'd have thought it would - the DICompileUnit doesn't carry it directly, which is fair. But I think it's more efficient to use the CUMap to go from DICompileUnit -> DwarfCompileUnit, rather than going from DIE->unit DIE-> DwarfCompileUnit?

AccelDebugNames.addName(Ref, Die, *CU);
break;
}
case AccelTableKind::Default:
llvm_unreachable("Default should have already been resolved.");
case AccelTableKind::None:
Expand Down