This is an archive of the discontinued LLVM Phabricator instance.

[AsmWriter] Properly handle uselistorder for global symbols
ClosedPublic

Authored by nikic on Jun 26 2021, 7:22 AM.

Details

Summary

Currently, AsmWriter will stick uselistorder directives for global values inside individual functions. This doesn't make a lot of sense, and interacts badly with D104950, as use list order adjustments will be performed while still working on a forward reference.

This patch instead always prints uselistorder directives for globals at the module level. This isn't really compatible with the previously used implementation approach. Rather than walking through all values again, use the OrderMap (after stabilizing its order) to go through all values and compute the use list shuffles for them. Classify them per-function, or nullptr for globals.

Even independently of D104950, this seems to fix a few verify-uselistorder failures. Conveniently, there is even a pre-existing failing test that this fixes.

Diff Detail

Event Timeline

nikic created this revision.Jun 26 2021, 7:22 AM
nikic requested review of this revision.Jun 26 2021, 7:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2021, 7:22 AM
dexonsmith accepted this revision.Jun 28 2021, 1:18 PM

Thanks! This is a nice approach; LGTM with a couple of minor nits.

Currently, AsmWriter will stick uselistorder directives for global values inside individual functions.

FWIW (not much I think), I vaguely remember a desire (mine? reviewers?) to avoid top-level uselistorder directives... but I guess at some point the dam broke. This is easier to reason about.

llvm/lib/IR/AsmWriter.cpp
241

Nit: this could be inverted with a continue to reduce nesting.

244

Nit: this could inverted with a continue to reduce nesting.

llvm/test/Bitcode/use-list-order2.ll
2

Looks like this was disabled to "fix" PR24755, which was introduced by the refactor of Function operands... a bit unfortunate for the test to have been turned off instead of reverting the patch that broke it (this approach seems to defeat the point of adding tests?)...

This revision is now accepted and ready to land.Jun 28 2021, 1:18 PM

perhaps I'm reading this wrong, but could we add a test for a global that previously had its uselistorder written in a function but is now written at the module level?

nikic updated this revision to Diff 355017.Jun 28 2021, 1:29 PM
nikic marked 2 inline comments as done.

Address style suggestions.

perhaps I'm reading this wrong, but could we add a test for a global that previously had its uselistorder written in a function but is now written at the module level?

Yeah, that seems like a good idea.

nikic updated this revision to Diff 355023.Jun 28 2021, 1:40 PM

Add additional test. I'm not sure if this is what you had in mind. This test shows how the previous representation that stores info in individual functions gets converted into the module level form now.

dexonsmith added inline comments.Jun 28 2021, 1:47 PM
llvm/test/Assembler/uselistorder_global.ll
1 ↗(On Diff #355023)

Might as well add a RUN: verify-uselistorder %s?

4 ↗(On Diff #355023)

My thought was that this would check that uselistorder is emitted in the "right" place:

// Check that global uselistorders are emitted at the top-level.
CHECK: define void @func1() {
CHECK: }
CHECK: define void @func2() {
CHECK: }
CHECK: uselistorder @g, {3, 2, 0, 1}

Probably not important to test that we can parse the directives in the "old" place. You could probably start with exact textual IR you want to see pop out the other side.

@aeubanks , WDYT?

aeubanks added inline comments.Jun 28 2021, 1:48 PM
llvm/test/Assembler/uselistorder_global.ll
4 ↗(On Diff #355023)

yeah, that's exactly what I was thinking

aeubanks accepted this revision.Jun 28 2021, 1:54 PM

otherwise lgtm

nikic updated this revision to Diff 355033.Jun 28 2021, 1:59 PM

Update test. Does this match what you had in mind?

dexonsmith accepted this revision.Jun 28 2021, 2:00 PM

Update test. Does this match what you had in mind?

LGTM.

lgtm
should BitcodeWriter also be updated?

lgtm
should BitcodeWriter also be updated?

I think if verify-uselistorder is passing on the use-list-order2.ll test, then the bitcode part is already working.

This revision was landed with ongoing or failed builds.Jun 28 2021, 2:15 PM
This revision was automatically updated to reflect the committed changes.

Currently, AsmWriter will stick uselistorder directives for global values inside individual functions.

FWIW (not much I think), I vaguely remember a desire (mine? reviewers?) to avoid top-level uselistorder directives... but I guess at some point the dam broke. This is easier to reason about.

In case you're interested, more of the history just paged itself in for me. IIRC, an early design for this directive named the users directly, requiring a directive in each function that had locals that used a global value. The final design, as you've seen, uses a shuffle vector based on predicting the "natural" order in the reader. This can live quite happily at the global scope (as you've done in this patch).