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).
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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) |
Made ChunksToKeep a const ref in function params & moved DeltaPasses cl option to main cpp file
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) |
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 |
Added more targeted tests & fixed off-by-one variable init. on ReduceFunctions.cpp and ReduceGlobalVars.cpp
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)
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? |
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. |
Pass doesn't consider debug functions anymore since D66257 takes care of them.
Simplified test so there's less IR noise
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? |
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).
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.)
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.
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.
this method returns nothing