[PATCH] D14043: [bugpoint] Add a named metadata (+their operands) reducer

[Keno Fischer via llvm-commits]

loladiro added inline comments.

================
Comment at: tools/bugpoint/CrashDebugger.cpp:530
@@ +529,3 @@
+  ValueToValueMapTy VMap;
+  Module *M = CloneModule(BD.getProgram(), VMap);
+
----------------
reames wrote:
> Please use a shared_ptr or something.  Raw pointers should be avoided.
I copied that from above. Happy to change the API throughout, but that should be a separate change.

================
Comment at: tools/bugpoint/CrashDebugger.cpp:549
@@ +548,3 @@
+  // delete them all at once to avoid invalidating the iterator
+  std::vector<NamedMDNode *> ToDelete;
+  for (auto &NamedMD : M->named_metadata())
----------------
reames wrote:
> Possibly: ToDelete.reserve(NamedMD.size())?
Sure

================
Comment at: tools/bugpoint/CrashDebugger.cpp:615
@@ +614,3 @@
+  // these dropped during cloning.
+  for (auto &NamedMD : BD.getProgram()->named_metadata()) {
+    // Drop the old one and create a new one
----------------
reames wrote:
> Do you need to worry about iterator invalidation here?
> 
> Would it be easier to just remove the operands or set them to undef?
No, the iterators are in the original module, but the ones we're dropping are in the new one. I didn't see an obvious way to just remove operands. I agree that would be preferable.

================
Comment at: tools/bugpoint/CrashDebugger.cpp:816
@@ +815,3 @@
+
+  if (!BugpointIsInterrupted) {
+    // Try to reduce the amount of global metadata (particularly debug info),
----------------
reames wrote:
> Please add an option to easily disable these passes.
Will do.

================
Comment at: tools/bugpoint/CrashDebugger.cpp:821
@@ +820,3 @@
+    std::vector<std::string> NamedMDNames;
+    for (auto &NamedMD : BD.getProgram()->named_metadata())
+      NamedMDNames.push_back(NamedMD.getName().str());
----------------
reames wrote:
> insert(end, md_begin, md_end)
Needs to convert from named md to its name


Repository:
  rL LLVM

http://reviews.llvm.org/D14043