This is an archive of the discontinued LLVM Phabricator instance.

[llvm-reduce] Add support for LTO bitcode files
ClosedPublic

Authored by ormris on Jun 6 2022, 5:19 PM.

Details

Summary

Adds support for reading and writing LTO bitcode files. While this feature can be replicated by stripping and regenerating summaries via opt, this patch makes reducing LTO bitcode a transparent process with no extra boiler-plate. This is especially useful when split LTO units are needed, as the process to generate them can be less than obvious.

Diff Detail

Event Timeline

ormris created this revision.Jun 6 2022, 5:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 5:19 PM
ormris requested review of this revision.Jun 6 2022, 5:19 PM
ormris updated this revision to Diff 434649.Jun 6 2022, 5:22 PM

Add context

Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 5:22 PM
arsenm added inline comments.Jun 6 2022, 5:30 PM
tools/llvm-reduce/ReducerWorkItem.cpp
425 ↗(On Diff #434646)

Single quotes around newline

434 ↗(On Diff #434646)

Why is this returning a default constructed ReducerWorkItem instead of null? I see some other places are doing this but I don't see how this makes sense

441 ↗(On Diff #434646)

Why does an IR reduction need to initialize target info?

455 ↗(On Diff #434646)

Why this change?

tools/llvm-reduce/llvm-reduce.cpp
121 ↗(On Diff #434646)

Do you really need a pass manager (a legacy one at that) just to write out the file? Why isn't there a simple function to use?

137 ↗(On Diff #434646)

Other places propagate the tool name from argv

144 ↗(On Diff #434646)

Ditto

ormris added inline comments.Jun 15 2022, 10:56 AM
tools/llvm-reduce/ReducerWorkItem.cpp
425 ↗(On Diff #434646)

Fixed.

434 ↗(On Diff #434646)

I opted for this to stay consistent with the surrounding code. Nonetheless, nullptr is more efficient at the same task, so I'll go ahead and use that instead.

441 ↗(On Diff #434646)

The ThinLTOBitcodeWriter pass depends on target info. It crashes without it.

455 ↗(On Diff #434646)

That change is no longer needed. I'll remove it.

tools/llvm-reduce/llvm-reduce.cpp
121 ↗(On Diff #434646)

Unfortunately, there isn't. If you look at opt, you'll see the same strategy.

137 ↗(On Diff #434646)

Fixed. I've added the tool name to the TestRunner class, so that it's accessible.

144 ↗(On Diff #434646)

See above.

ormris updated this revision to Diff 437250.Jun 15 2022, 10:57 AM
  • Add toolname to errors
  • Remove unnecessary changes
arsenm accepted this revision.Jun 29 2022, 4:16 PM

While I don't understand the LTO bitcode files, the reduce changes seem fine

llvm/tools/llvm-reduce/deltas/Delta.cpp
64–66

Don't need llvm:: and these look like they need line wrapping

This revision is now accepted and ready to land.Jun 29 2022, 4:16 PM
This revision was landed with ongoing or failed builds.Jun 30 2022, 8:58 AM
This revision was automatically updated to reflect the committed changes.

Sounds good. Thanks for the review.