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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

sorry for the slow review

do you have cases where stripping the debug info `e.g. passing the IR through `opt -passes=strip` makes the IR uninteresting, but this significantly helps over just the `metadata` pass?



================
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.
+;
----------------
looks copy/pasted


================
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
----------------
could you add `--abort-on-invalid-reduction`?


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


================
Comment at: llvm/tools/llvm-reduce/DeltaManager.cpp:70
     DELTA_PASS("metadata", reduceMetadataDeltaPass)                            \
+    DELTA_PASS("dimetadata", reduceDIMetadataDeltaPass)                        \
     DELTA_PASS("arguments", reduceArgumentsDeltaPass)                          \
----------------
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


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


================
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)
----------------
nit: formatting
I see some other unformatted code, did you run clang-format?


================
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});
----------------
check if `MDT` is null?


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceDIMetadata.cpp:64
+
+    Visited.insert(MD);
+  }
----------------
should this be at the top of the loop?


================
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.
----------------
with C++17 we should be able to use `auto [a, b, c] = foo;`


================
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()) {
----------------
use brackets for bodies with more than one line


================
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(), {}));
+  }
----------------
is it possible for multiple DINodes to refer to a 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));
+  }
----------------
check if this is null?


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

https://reviews.llvm.org/D132077