[PATCH] D132077: [llvm-reduce] Add pass that reduces DebugInfo metadata

[Matthew Voss via Phabricator via llvm-commits]

ormris added a comment.

Everything should be addressed. Sorry for the delay. Things have been busy over here.

In D132077#3765732 <https://reviews.llvm.org/D132077#3765732>, @aeubanks wrote:

> actually I just happened to run across a case where this would be useful, but I'm getting tons of verifier errors in reduction to do with `invalid template parameter`

Yes, this does lean on the verifier. It's fairly blind to the structure of DI Metadata. Did it provide a useful reduction?



================
Comment at: llvm/test/tools/llvm-reduce/remove-dimetadata.ll:1-2
+; Test that llvm-reduce can remove uninteresting metadata from an IR file.
+; The Metadata pass erases named & unnamed metadata nodes.
+;
----------------
aeubanks wrote:
> looks copy/pasted
Fixed


================
Comment at: llvm/test/tools/llvm-reduce/remove-dimetadata.ll:4
+;
+; RUN: llvm-reduce --delta-passes=dimetadata --test %python --test-arg %p/Inputs/remove-dimetadata.py %s -o %t
+; RUN: FileCheck <%t %s
----------------
aeubanks wrote:
> could you add `--abort-on-invalid-reduction`?
Fixed


================
Comment at: llvm/tools/llvm-reduce/DeltaManager.cpp:72
+    DELTA_PASS("metadata", reduceMetadataDeltaPass)                            \
+    DELTA_PASS("dimetadata", reduceDIMetadataDeltaPass)                        \
     DELTA_PASS("simplify-instructions", simplifyInstructionsDeltaPass)         \
----------------
aeubanks wrote:
> can this be `di-metadata`?  `dimetadata` is too squished for me
Fixed


================
Comment at: llvm/tools/llvm-reduce/DeltaManager.cpp:70
     DELTA_PASS("metadata", reduceMetadataDeltaPass)                            \
+    DELTA_PASS("dimetadata", reduceDIMetadataDeltaPass)                        \
     DELTA_PASS("arguments", reduceArgumentsDeltaPass)                          \
----------------
aeubanks wrote:
> ormris wrote:
> > aeubanks wrote:
> > > I wonder if we should move the metadata passes later where we should already have removed as many potential metadata havers, e.g. after `instructions` or `ir-passes`
> > Good point. I've run some tests locally and it seems like moving it after the instructions pass saves a few seconds. I'll change the patch.
> > 
> > | Position | Time (s) |
> > | Current | 85.3 |
> > | Last | 81.8 |
> > | After instructions | 80.96 |
> running `dimetadata` before `metadata` seems better since `dimetadata` can make `metadata` do less work, but not vice versa
Fixed


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:45-46
+  while (!ToLook.empty()) {
+    MDNode *MD = ToLook.top();
+    ToLook.pop();
+
----------------
aeubanks wrote:
> making `ToLook` a `vector` and using `pop_back` here seems nicer
FIxed


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:50
+    if (MD && (isa<DINode>(MD) || isa<DILocation>(MD)))
+      //Scan operands and record attached tuples
+      for (size_t I = 0; I < MD->getNumOperands(); ++I)
----------------
aeubanks wrote:
> nit: formatting
> I see some other unformatted code, did you run clang-format?
I did not. Should be fixed.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:53
+        if (MDTuple *MDT = dyn_cast_or_null<MDTuple>(MD->getOperand(I)))
+          if (!Visited.count(MDT) && MDT->getNumOperands())
+            Tuples.insert({MD,I,MDT});
----------------
aeubanks wrote:
> check if `MDT` is null?
Fixed


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:64
+
+    Visited.insert(MD);
+  }
----------------
aeubanks wrote:
> should this be at the top of the loop?
No, that messes with checks in the body of the loop. There are two places where if check if the node is visited.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:70
+    size_t Z = 0, J = 0;
+    std::tie(MO, J, MT) = T;
+    // Null the operands of the tuple that are not in the desired chunks.
----------------
aeubanks wrote:
> with C++17 we should be able to use `auto [a, b, c] = foo;`
Fixed.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:74
+      // Ignore any operands that are not DebugInfo metadata nodes.
+      if (MDNode *OMD = dyn_cast_or_null<DINode>(MT->getOperand(I)))
+        if (!O.shouldKeep()) {
----------------
aeubanks wrote:
> use brackets for bodies with more than one line
Fixed.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:83-84
+    // replace it with a empty tuple.
+    if (MT->getNumOperands() == Z)
+      MO->replaceOperandWith(J, MT->get(MT->getContext(), {}));
+  }
----------------
aeubanks wrote:
> is it possible for multiple DINodes to refer to a tuple?
Yes. In my example IR files, multiple nodes refer to the same empty tuple.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:96
+    for (Instruction &I : instructions(F))
+      MDs.push_back(I.getMetadata(llvm::LLVMContext::MD_dbg));
+  }
----------------
aeubanks wrote:
> check if this is null?
Fixed


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

https://reviews.llvm.org/D132077