Page MenuHomePhabricator

[Bugpoint redesign] Added pass to reduce Metadata
ClosedPublic

Authored by diegotf on Jul 19 2019, 3:34 PM.

Details

Summary

This pass tries to reduce named & unnamed metadata nodes from an IR file, as well as any declared debug functions (as well as their calls).

Diff Detail

Repository
rL LLVM

Event Timeline

diegotf created this revision.Jul 19 2019, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 3:35 PM
diegotf updated this revision to Diff 210919.Jul 19 2019, 3:39 PM

Re-worded file header comment and added newline to ReduceMetadata.cpp

dblaikie added inline comments.Jul 22 2019, 3:10 PM
llvm/tools/llvm-reduce/DeltaManager.h
19–40 ↗(On Diff #210919)

Anonymous namespaces and static variables aren't suitable for header files (they can lead to ODR violations, etc) You can inline these things inside runDeltaPasses (using lambdas for local functions, etc) or take the runDeltaPasses definition out of line into a .cc file and move these static variables/anonymous namespace content to there.

llvm/tools/llvm-reduce/deltas/ReduceGlobalVars.cpp
15 ↗(On Diff #210919)

Might've been a bit easier to do some of these renames as separate patches - super easy to review (or commit without review) & keeps the "interesting" reviews uncluttered.

llvm/tools/llvm-reduce/deltas/ReduceMetadata.cpp
20 ↗(On Diff #210919)

Is this intentionally passed by value for some reason? (usually a non-trivial data structure like a std::vector would be passed by const ref if a copy isn't needed, etc)

diegotf updated this revision to Diff 211219.Jul 22 2019, 3:40 PM
diegotf marked 6 inline comments as done.

Made ChunksToKeep a const ref in function params & moved DeltaPasses cl option to main cpp file

diegotf added inline comments.Jul 22 2019, 3:41 PM
llvm/tools/llvm-reduce/DeltaManager.h
19–40 ↗(On Diff #210919)

I don't know why I keep forgetting this, I swear I'll tattoo it on my arm next time I forget haha.

Thanks for the comment, David.

llvm/tools/llvm-reduce/deltas/ReduceGlobalVars.cpp
15 ↗(On Diff #210919)

My bad, I'll do that from now on :)

llvm/tools/llvm-reduce/deltas/ReduceMetadata.cpp
20 ↗(On Diff #210919)

Good catch, I forgot to pass it as const ref.

I'll make another patch later with the same fix on other Delta Passes to reduce the clutter on this diff

The goal of this pass is to be able to remove as many intrinsic calls and metadata (debug info and non-debug info metadata alike) while still preserving the "interestingness". I would figure maybe some more testing would be appropriate - since this currently only tests (if I understand the test) that it can remove all of these things - not that it preserves any of them at any time? Perhaps some tests for certain interesting metadata and intrinsics to ensure some are preserved?

(also, if the goal is to make a more streamlined reduction tool - maybe it'd be good to avoid adding a new command line argument/tweakable knob? & have more targeted tests/interestingness test? (to make tests more legible, I wonder if it'd be handy to keep the interestingness test inside the test - in the form of catting it out to a file from the .ll test file?))

llvm/tools/llvm-reduce/llvm-reduce.cpp
56–76 ↗(On Diff #211219)

Is this idiomatic? (are there other uses of cl options that use a similar structure (indirecting through cl::location, then storing to another variable?) - I just haven't seen it done that way & wonder if there's some other cl::opt usage that would be more suitable/common)

diegotf marked an inline comment as done.Jul 22 2019, 5:38 PM

The goal of this pass is to be able to remove as many intrinsic calls and metadata (debug info and non-debug info metadata alike) while still preserving the "interestingness". I would figure maybe some more testing would be appropriate - since this currently only tests (if I understand the test) that it can remove all of these things - not that it preserves any of them at any time? Perhaps some tests for certain interesting metadata and intrinsics to ensure some are preserved?

That's very good insight, I'll modify the test so it verifies interesting metadata is preserved.

(also, if the goal is to make a more streamlined reduction tool - maybe it'd be good to avoid adding a new command line argument/tweakable knob? & have more targeted tests/interestingness test? (to make tests more legible, I wonder if it'd be handy to keep the interestingness test inside the test - in the form of catting it out to a file from the .ll test file?))

I thought about the complexity the run-only might add, but since this tool will probably be integrated into the bugpoint tool itself, the option will mainly be used for testing purposes. Although I agree that tests need to be more targeted, and tackling this might solve the need for the former.

llvm/tools/llvm-reduce/llvm-reduce.cpp
56–76 ↗(On Diff #211219)

I found it quite odd as well, and I took it straight from llc.cpp (line 163).

But now that you mention it, I think I'll move the struct's behaviour to a helper function, since it adds unnecessary complexity

diegotf updated this revision to Diff 213782.Aug 6 2019, 7:23 PM

Added more targeted tests & fixed off-by-one variable init. on ReduceFunctions.cpp and ReduceGlobalVars.cpp

diegotf updated this revision to Diff 214235.Aug 8 2019, 2:29 PM

Changed pass to reflect changes in base diff, moved interesting-ness test to python & changed test so it only reduces certain metadata (previously it just stripped everything)

diegotf updated this revision to Diff 214236.Aug 8 2019, 2:31 PM

Added missing check in test

diegotf edited the summary of this revision. (Show Details)Aug 8 2019, 2:32 PM
diegotf updated this revision to Diff 214259.Aug 8 2019, 4:27 PM
diegotf edited the summary of this revision. (Show Details)

Removed some stale changes that have since been integrated into base

dblaikie added inline comments.Aug 14 2019, 1:15 PM
llvm/test/Reduce/remove-metadata.ll
1 ↗(On Diff #214259)

Would it be possible to test this without specifically debug info metadata? If the feature is general over all metadata it might be clearer to demonstrate with less IR (fewer metadata nodes, if they don't have to form a complete debug info metadata graph, etc).

llvm/tools/llvm-reduce/deltas/ReduceMetadata.cpp
10–11 ↗(On Diff #214259)

Any reason debug functions would need to be a special case here, rather than handled as a consequence of a general pass that removes any functions?

diegotf marked 3 inline comments as done.Aug 14 2019, 2:23 PM
diegotf added inline comments.
llvm/test/Reduce/remove-metadata.ll
1 ↗(On Diff #214259)

Agreed, I'll modify the test so it only contains fewer unnamed metadata nodes (and no !dbg metadata)

llvm/tools/llvm-reduce/deltas/ReduceMetadata.cpp
10–11 ↗(On Diff #214259)

Yeah, since ReduceFunctions.cpp only considers defined functions, they would be ignored. Though I've been thinking about changing that lately...

I'll cook up a quick diff so this pass doesn't have to consider debug functions.

diegotf updated this revision to Diff 215269.Aug 14 2019, 3:23 PM
diegotf marked an inline comment as done.

Pass doesn't consider debug functions anymore since D66257 takes care of them.

Simplified test so there's less IR noise

dblaikie added inline comments.Aug 14 2019, 3:31 PM
llvm/test/Reduce/remove-metadata.ll
8–24 ↗(On Diff #215269)

Could you change all of this metadata (both named and unnamed) to be more "arbitrary" rather than special llvm things - use some metasyntactic variable names (foo/bar/baz, etc) and only include as much as is needed to test the code? (so I guess only two named (one that's kept, one that's dropped) and two unnamed nodes)?

Probably actively test /for/ the presence of the things that remain (and a CHECK-NOT: ! after it, just to be "and no other nodes after this" rather than having to explain why 3 is the right number)

llvm/tools/llvm-reduce/deltas/ReduceMetadata.cpp
60 ↗(On Diff #215269)

This is for removing metadata from functions, right? But there's no testing for metadata on functions anymore - does that testing need to be added?

diegotf updated this revision to Diff 215276.Aug 14 2019, 4:02 PM
diegotf marked 4 inline comments as done.

Simplified test & Added check to verify pass removes metadata from globals, functions & instructions.

llvm/test/Reduce/remove-metadata.ll
8–24 ↗(On Diff #215269)

done!

llvm/tools/llvm-reduce/deltas/ReduceMetadata.cpp
60 ↗(On Diff #215269)

Yes, let me add a little function to the test

diegotf updated this revision to Diff 215490.Aug 15 2019, 4:03 PM

Updated test to output to STDOUT, instead of reduced.ll

From the discussion on https://reviews.llvm.org/D66110 - would it be possible/good to rewrite the "correctness" script to be a one-size-fits-all - diffing against a golden file of expected output (& maybe somehow deriving the golden file name from the original file name - which might be best achieved by copying both files to a temp directory)?

Like I said on that other review, I'm understanding of hesitance around golden files - and in this case it means another file to do IR upgrades on when LLVM textual IR changes (but most of those are done en-mass, so I don't think having twice the number of files for these tests is particularly problematic - there are already lots of other .ll files in Inputs directories around LLVM's tests, so anyone doing mass migration would already be scanning/patching them).

By doing it this way, it'll be easier to read in some ways - comparing one IR file to another (diffing, for instance). No need to complicate the interestingness test with novel ways to preserve features that aren't under test but are needed for the test (like preserving certain functions when you're trying to test that the metadata is removed from within the function).

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Sep 10, 3:12 PM
Closed by commit rL371562: llvm-reduce: Add pass to reduce Metadata (authored by dblaikie, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
xbolva00 added inline comments.
llvm/trunk/tools/llvm-reduce/deltas/ReduceMetadata.cpp
54

this method returns nothing

The test added in this change has a file leak that causes all possible temp file patterns to be created, at which point it starts failing, see https://bugs.llvm.org/show_bug.cgi?id=43278 (Diego, I tried to cc you, but bugzilla didn't know your email address.) Can you please take a look, and revert if it takes a while to investigate? (remove-funcs seems to have the same problem.)

The test added in this change has a file leak that causes all possible temp file patterns to be created, at which point it starts failing, see https://bugs.llvm.org/show_bug.cgi?id=43278 (Diego, I tried to cc you, but bugzilla didn't know your email address.) Can you please take a look, and revert if it takes a while to investigate? (remove-funcs seems to have the same problem.)

All the tests work roughly the same way, so I expect they're all broken in roughly the same way.

I'll look into it/them. Might disable the tests in the short term to keep the buildbots happy.

w2yehia added a subscriber: w2yehia.EditedWed, Sep 11, 11:48 AM

Just FYI, that our (IBM's) internal bots are also breaking because the following tests are not cleaning the build/test/Reduce/tmp folder:

LLVM :: Reduce/remove-funcs.ll
LLVM :: Reduce/remove-metadata.ll

And we're forced to sometimes do a clean build.

Just FYI, that our (IBM's) internal bots are also breaking because the following tests are not cleaning the build/test/Reduce/tmp folder:

LLVM :: Reduce/remove-funcs.ll
LLVM :: Reduce/remove-metadata.ll

And we're forced to sometimes do a clean build.

The test added in this change has a file leak that causes all possible temp file patterns to be created, at which point it starts failing, see https://bugs.llvm.org/show_bug.cgi?id=43278 (Diego, I tried to cc you, but bugzilla didn't know your email address.) Can you please take a look, and revert if it takes a while to investigate? (remove-funcs seems to have the same problem.)

Fixed in r371696 - thanks for the reports/identifying the issue!