This is an archive of the discontinued LLVM Phabricator instance.

[llvm-reduce] Remove dso_local when possible
ClosedPublic

Authored by swamulism on Mar 15 2021, 5:30 PM.

Details

Summary

Add a new delta pass to llvm-reduce that removes dso_local when possible

Diff Detail

Event Timeline

swamulism created this revision.Mar 15 2021, 5:30 PM
swamulism requested review of this revision.Mar 15 2021, 5:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 5:30 PM

We are indeed missing reductions of a number of such things, but isn't this a bit too specialized?
I don't know what the solution is (and that is why i didn't touch it), but i'd be hoping for something more generic..

fhahn added a comment.Mar 16 2021, 5:09 AM

We are indeed missing reductions of a number of such things, but isn't this a bit too specialized?
I don't know what the solution is (and that is why i didn't touch it), but i'd be hoping for something more generic..

We should probably also try to reduce the linkage & visibility. Although it is not completely clear what 'reduced'/'simpler' means here. But I guess it might make sense to try to set them to the default values? Is this the direction you meant with 'more generic' or reducing other bits of globals as well?

We are indeed missing reductions of a number of such things, but isn't this a bit too specialized?
I don't know what the solution is (and that is why i didn't touch it), but i'd be hoping for something more generic..

We should probably also try to reduce the linkage & visibility. Although it is not completely clear what 'reduced'/'simpler' means here. But I guess it might make sense to try to set them to the default values? Is this the direction you meant with 'more generic' or reducing other bits of globals as well?

We also miss nuw/nsw/inbounds/exact and likely others.

We are indeed missing reductions of a number of such things, but isn't this a bit too specialized?
I don't know what the solution is (and that is why i didn't touch it), but i'd be hoping for something more generic..

We should probably also try to reduce the linkage & visibility. Although it is not completely clear what 'reduced'/'simpler' means here. But I guess it might make sense to try to set them to the default values? Is this the direction you meant with 'more generic' or reducing other bits of globals as well?

When I'm reducing codegen problems, I tend to make the linkage and visibility stricter since it ends up with fewer machine instructions

The only way to remove dso_local is via GlobalValue::setDSOLocal(), I don't think there is some more generic way to do it.

When reducing, we should decide if we want to reduce to dso_local or the default (dso_preemptable), but we shouldn't just leave it be. Either way is fine to me, although I slightly prefer removing it since it makes the printed IR cleaner, even if that's technically less strict.

We also miss nuw/nsw/inbounds/exact and likely others.

We should also do these, but in a future patch.

Maybe we could have two llvm-reduce modes, one for adding as many restrictive qualifiers as possible, and one for making clean IR that uses defaults as much as possible. IMO more people would rather see defaults than restrictive qualifiers.

aeubanks accepted this revision.Mar 22 2021, 8:58 AM

Anyway, I think this looks good, we can always easily change the way we reduce in the future.

This revision is now accepted and ready to land.Mar 22 2021, 8:58 AM
fhahn added inline comments.Mar 22 2021, 9:06 AM
llvm/tools/llvm-reduce/deltas/ReduceGlobalValues.cpp
2

nit: this needs fixing?

11

can we say more specifically what this reduces? It only reduces some attributes of globals.

llvm/tools/llvm-reduce/deltas/ReduceGlobalValues.h
2

nit: fix formatting?

swamulism updated this revision to Diff 332473.Mar 22 2021, 4:44 PM
swamulism marked 3 inline comments as done.

Add header guard and format docs

separately, we should fix up all the headers in llvm-reduce, they're almost all missing include guards

swamulism updated this revision to Diff 332489.Mar 22 2021, 6:17 PM

Conform to clang-tidy standards

swamulism updated this revision to Diff 332502.Mar 22 2021, 7:53 PM

Actually conform to clang-tidy standards

This revision was landed with ongoing or failed builds.Mar 29 2021, 12:00 PM
This revision was automatically updated to reflect the committed changes.