[PATCH] D65026: [Bugpoint redesign] Added pass to reduce Metadata

[Diego Treviño via Phabricator via llvm-commits]

diegotf marked an inline comment as done.
diegotf added a comment.

In D65026#1596697 <https://reviews.llvm.org/D65026#1596697>, @dblaikie wrote:

> 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.



================
Comment at: llvm/tools/llvm-reduce/llvm-reduce.cpp:56-76
+namespace {
+std::vector<std::string> DeltaPasses;
+
+struct DeltaPassOption {
+  void operator=(const std::string &Val) const {
+    if (Val.empty())
+      return;
----------------
dblaikie wrote:
> 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)
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65026/new/

https://reviews.llvm.org/D65026