Page MenuHomePhabricator

[Bugpoint redesign] Added Pass to Remove Global Variables
AcceptedPublic

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

Details

Summary

This pass tries to remove Global Variables, as well as their derived uses. For example if a variable @x is used by %call1 and %call2, both these uses and the definition of @x are deleted. Moreover if %call1 or %call2 are used elsewhere those uses are also deleted, and so on recursively.

I'm still uncertain if this pass should remove derived uses, I'm open to discussion as to what might be the best approach.

This diff refactored how the Delta Debugging Algorithm is called in order to avoid confusion as to how it works.

It also fixes an out-of-range error when specialized passes got desired chunk-targets , and refactors the removal of uninteresting chunks in Delta.h, since the current implementation crashed when the last Chunk was erased.

Depends on D63672

Event Timeline

diegotf created this revision.Wed, Jul 3, 5:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 3, 5:24 PM
diegotf edited the summary of this revision. (Show Details)Wed, Jul 3, 5:25 PM
fhahn added a subscriber: fhahn.Fri, Jul 5, 7:26 AM
fhahn added inline comments.
llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.cpp
2

file name needs updating I think

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

file name needs updating I think

diegotf updated this revision to Diff 208491.Mon, Jul 8, 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.EditedMon, Jul 8, 6:57 PM

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

diegotf edited the summary of this revision. (Show Details)Mon, Jul 8, 7:09 PM
diegotf edited the summary of this revision. (Show Details)
dblaikie added inline comments.Thu, Jul 11, 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
54

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

llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.h
24–32

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.Thu, Jul 11, 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)Fri, Jul 12, 12:10 PM
diegotf updated this revision to Diff 209647.Fri, Jul 12, 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 .Fri, Jul 12, 5:11 PM
diegotf edited the summary of this revision. (Show Details)
diegotf updated this revision to Diff 209649.Fri, Jul 12, 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.Fri, Jul 12, 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
24–32

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.Mon, Jul 15, 1:36 PM
llvm/tools/llvm-reduce/deltas/RemoveGlobalVars.h
24–25

(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.Mon, Jul 15, 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.Mon, Jul 15, 2:43 PM
llvm/tools/llvm-reduce/deltas/Delta.h
67

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.Mon, Jul 15, 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

Yeah, I goofed up and uploaded a wrong commit

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

Added license & file header to cpp files

dblaikie accepted this revision.Mon, Jul 15, 3:53 PM
This revision is now accepted and ready to land.Mon, Jul 15, 3:53 PM