This is an archive of the discontinued LLVM Phabricator instance.

[Bugpoint redesign] Added Pass to Remove Global Variables
ClosedPublic

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

Event Timeline

diegotf created this revision.Jul 3 2019, 5:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 5:24 PM
diegotf edited the summary of this revision. (Show Details)Jul 3 2019, 5:25 PM
fhahn added a subscriber: fhahn.Jul 5 2019, 7:26 AM
fhahn added inline comments.
llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.cpp
1 ↗(On Diff #207934)

file name needs updating I think

llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.h
1 ↗(On Diff #207934)

file name needs updating I think

diegotf updated this revision to Diff 208491.Jul 8 2019, 1:14 PM
diegotf marked 2 inline comments as done.

Fixed top-level comments to fit actual filename & description.

Reworded doxygen comment on eraseDerivedUsers

diegotf updated this revision to Diff 208570.EditedJul 8 2019, 6:57 PM

Changed commit so it only has changes made to parent diff (D63672)

diegotf edited the summary of this revision. (Show Details)Jul 8 2019, 7:09 PM
diegotf edited the summary of this revision. (Show Details)
dblaikie added inline comments.Jul 11 2019, 4:43 PM
llvm/test/Reduce/remove-global-vars.ll
9–15

Might be stronger to:

CHECK-NOT: "global" on either side of the "CHECK: @interesting = global"

That way it'd be resilient to any reordering, for instance (where you could silently pass the test if LLVM IR happened to print 2, 3, unnumbered, 1)

llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.cpp
53 ↗(On Diff #208570)

Is the indentation off here? (clang-format can be handy to clean up stuff like this)

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

Classes to group static functions are somewhat to be avoided, I think - should this be a helper namespace, or maybe leaving the functions in the 'llvm' namespace while making the names more specific?

diegotf updated this revision to Diff 209377.Jul 11 2019, 4:58 PM
diegotf marked an inline comment as done.

Fixed out-of-range bug on specialized passes when getting Targets

Refactored removal of uninteresting chunks in Delta.h, since erasing them whilst iterating crashed when last Chunk was erased

Re-ran clang-format

diegotf edited the summary of this revision. (Show Details)Jul 12 2019, 12:10 PM
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
9–15

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
9–15

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

19–33

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
19–33

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

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
6

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!