Page MenuHomePhabricator

[Bugpoint redesign] Added Pass to Remove Global Variables
ClosedPublic

Authored by diegotf on Jul 3 2019, 5:24 PM.

Details

Summary

This pass tries to remove Global Variables as well as their direct uses, for example if a given GV @x is deemed uninteresting and it's used by %call and %call2. Both the GV as well as its calls would be deleted (any uses of the calls would be replaced with undef)

Depends on D63672

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
diegotf updated this revision to Diff 209647.Jul 12 2019, 5:09 PM
diegotf marked 2 inline comments as done.

Refactored Delta Debugging Algorithm from a template class to an inline function in LLVM namespace; the specialized behaviour called by the generic Delta pass is given as parameters to the function instead of being given as a class template and then called statically in order to reduce confusion.

Note: Specialized Delta passes are also now inline functions instead of classes that groupped static functions.

diegotf retitled this revision from [Bugpoint redesign] Added Pass to Remove Global Variables to [Bugpoint redesign] Added Pass to Remove Global Variables .Jul 12 2019, 5:11 PM
diegotf edited the summary of this revision. (Show Details)
diegotf updated this revision to Diff 209649.Jul 12 2019, 5:20 PM
diegotf marked 2 inline comments as done.
diegotf retitled this revision from [Bugpoint redesign] Added Pass to Remove Global Variables to [Bugpoint redesign] Added Pass to Remove Global Variables.

Re-worded header comments to fit new implementation

diegotf added inline comments.Jul 12 2019, 5:20 PM
llvm/test/Reduce/remove-global-vars.ll
8–14 ↗(On Diff #208570)

Good catch! I hadn't thought of that.

llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.h
23–31 ↗(On Diff #208570)

I've refactored the Generic Delta pass in order to avoid this problem. I believe it's also simpler and easier to read.

dblaikie added inline comments.Jul 15 2019, 1:36 PM
llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.h
23–24 ↗(On Diff #209649)

(non-member) Static functions aren't appropriate in header files (they lead to code size increases and ODR violations).

You could move these into some sort of implementation namespace (llvm::impl or something like that) or I guess they could be stateless lambdas - or moved out-of-line into a .cpp file instead. (moving the removeGlobalsDeltaPass definition out of line so it can refer to the implementation functions in the .cpp file (where those functions can/should be 'static' in that case))

diegotf updated this revision to Diff 209957.Jul 15 2019, 2:34 PM
diegotf marked an inline comment as done.

Moved Delta pass function definitions from header to cpp files in order to avoid ODR violations.

dblaikie added inline comments.Jul 15 2019, 2:43 PM
llvm/tools/llvm-reduce/deltas/Delta.h
67 ↗(On Diff #209957)

This 'inline' keyword probably needs to be removed. I'm surprised if this code links as-is (or compiles - some sort of warning/error like this:

inl.cpp:1:13: warning: inline function 'f' is not defined [-Wundefined-inline]
inline void f();

^

inl.cpp:3:3: note: used here

f();
^
diegotf updated this revision to Diff 209972.Jul 15 2019, 3:22 PM
diegotf marked 2 inline comments as done.

Removed inline keyword from function declarations (since it's defined out-of-line)

llvm/tools/llvm-reduce/deltas/Delta.h
67 ↗(On Diff #209957)

Yeah, I goofed up and uploaded a wrong commit

diegotf updated this revision to Diff 209978.Jul 15 2019, 3:48 PM

Added license & file header to cpp files

dblaikie accepted this revision.Jul 15 2019, 3:53 PM
This revision is now accepted and ready to land.Jul 15 2019, 3:53 PM
diegotf updated this revision to Diff 212707.Jul 31 2019, 5:19 PM

Changed Pass behavior so it doesnt delete derived uses anymore.

diegotf edited the summary of this revision. (Show Details)Jul 31 2019, 5:19 PM
diegotf updated this revision to Diff 213667.Aug 6 2019, 11:25 AM

Re-added behavior to delete direct uses opf GVs (accidentally deleted)

diegotf updated this revision to Diff 213683.Aug 6 2019, 12:13 PM

Remove Metadata pass from DeltaManager.h and CMakeLists.txt

diegotf updated this revision to Diff 213690.Aug 6 2019, 12:40 PM

Update diff so it only show changes made to D63672

diegotf updated this revision to Diff 213759.Aug 6 2019, 5:28 PM

Changed GV unit test to fit to a more targeted interesting-ness test

diegotf updated this revision to Diff 213761.Aug 6 2019, 5:30 PM

Changed it to show only changes to D63672

Harbormaster completed remote builds in B36284: Diff 213761.
This revision was automatically updated to reflect the committed changes.

hmm, hard to review - the baseline review (D63672) seems inconsistent with the API usage here - so I'm not sure what's authoritative, which design changes have been discussed/finalized, etc..

diegotf reopened this revision.Aug 8 2019, 11:37 AM

hmm, hard to review - the baseline review (D63672) seems inconsistent with the API usage here - so I'm not sure what's authoritative, which design changes have been discussed/finalized, etc..

I'll update this (and the child diffs) to fit the changes in baseline

This revision is now accepted and ready to land.Aug 8 2019, 11:37 AM
diegotf updated this revision to Diff 214200.Aug 8 2019, 11:39 AM

Formatting changes & added llvm-reduce as dependency to tests

diegotf updated this revision to Diff 214207.Aug 8 2019, 12:22 PM

Update diff to reflect changes on baseline & moved interesting-ness test to python

diegotf edited the summary of this revision. (Show Details)Aug 8 2019, 12:23 PM
diegotf updated this revision to Diff 214254.Aug 8 2019, 4:14 PM

General Formatting

dblaikie added inline comments.Aug 14 2019, 12:16 PM
llvm/test/Reduce/remove-global-vars.ll
8–14 ↗(On Diff #208570)

seems good to have it on both sides (CHECK-NOT before and after the CHECK @interesting).

18–32 ↗(On Diff #214254)

Should these CHECK-NOTs also have some CHECKs that demonstrate what these have been transformed into? (showing the undefs appear)

Also - are there differences between uninteresting1/2/3 that's interesting/important to testing here, or could there be similar test coverage with 1 uninteresting/removed global?

dblaikie added inline comments.Aug 14 2019, 12:18 PM
llvm/tools/llvm-reduce/deltas/Delta.cpp
123–126 ↗(On Diff #214254)

This change appears independent of the rest of this patch - if that's the case, please commit it separately. (no need to send it for review if it's nothing too interesting - if it's just reducing the scope of variables as is generally good)

diegotf updated this revision to Diff 215222.Aug 14 2019, 1:19 PM
diegotf marked 4 inline comments as done.

Simplified test & added check to verify that direct uses are replaced with undef

diegotf added inline comments.Aug 14 2019, 1:19 PM
llvm/test/Reduce/remove-global-vars.ll
18–32 ↗(On Diff #214254)

Agreed, i'll add the checks. And yeah, uninteresting 2&3 might be overkill. I'll update the test accordingly

llvm/tools/llvm-reduce/deltas/Delta.cpp
123–126 ↗(On Diff #214254)

I'll move it to another then :)

dblaikie accepted this revision.Aug 14 2019, 1:23 PM

Looks good - thanks (:

diegotf updated this revision to Diff 215239.Aug 14 2019, 1:59 PM

Re-ran clang-format

Closed by commit rGd1ffcd07456b: [Bugpoint redesign] Added Pass to Remove Global Variables (authored by Diego Trevino Ferrer <diegof30@gmail.com>). · Explain WhyAug 14 2019, 2:01 PM
This revision was automatically updated to reflect the committed changes.

Surprisingly test remove-global-vars.ll fails on buildbot

http://lab.llvm.org:8011/builders/lld-x86_64-ubuntu-fast/builds/4649

Previous build was OK and this test case worked.

Either this testcase is unstable or something was miscompiled after
[X86] Add custom type legalization for bitcasting mmx to v2i32/v4i16/v8i8 to use movq2dq instead of going through memory.

cc @craig.topper

xbolva00 added inline comments.Aug 15 2019, 12:15 PM
llvm/test/Reduce/remove-global-vars.ll
5 ↗(On Diff #215240)

This looks weird

diegotf reopened this revision.Aug 15 2019, 3:09 PM

Patch here: D66314

This revision is now accepted and ready to land.Aug 15 2019, 3:09 PM
diegotf updated this revision to Diff 215481.Aug 15 2019, 3:23 PM

Changed test so reduced version is printed to STDOUT

This revision was automatically updated to reflect the committed changes.

Hi,

Bots warn;
srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/llvm-reduce/deltas/ReduceGlobalVars.cpp:17:6: warning: '@returns' command used in a comment that is attached to a function returning void [-Wdocumentation]
1 warning generated.

Can you fix this small issue? Thanks

Hi,

Bots warn;
srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/llvm-reduce/deltas/ReduceGlobalVars.cpp:17:6: warning: '@returns' command used in a comment that is attached to a function returning void [-Wdocumentation]
1 warning generated.

Can you fix this small issue? Thanks

@bkramer fixed this in r369848 - thanks Ben!