[PATCH] D19034: PR27284 - Reverse the ownership between DICompileUnit and DISubprogram

[David Blaikie via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Generally looks good - bunch of cleanup sort of stuff & a few casual questions, none of which I think would block this being committed.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1983-1985
@@ +1982,5 @@
+        if (auto *SPs = dyn_cast_or_null<MDTuple>(CU_SP.second))
+        for (auto &Op : SPs->operands())
+          if (auto *SP = dyn_cast_or_null<MDNode>(Op))
+            SP->replaceOperandWith(7, CU_SP.first);
+
----------------
Indentation (looks like the for SPs loop is indented at the same level as the if CU_SP.second condition - though it appears as though it's semantically nested inside it)

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2255
@@ -2246,1 +2254,3 @@
+      if (Metadata *SPs = getMDOrNull(Record[11]))
+        CUSubprograms.push_back({CU, SPs});
       break;
----------------
Can't remember if this braced init works in MSVC, you might end up needing to use make_pair

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2266
@@ +2265,3 @@
+      Metadata *CUorFn = getMDOrNull(Record[15]);
+      unsigned Offset = Record.size() == 19 ? 1 : 0;
+      bool HasFn = Offset && dyn_cast_or_null<ConstantAsMetadata>(CUorFn);
----------------
You could drop the conditional operator here, if you like - booleans convert to 1 and 0. Up to you if you think that's sufficiently/more readable, etc. (could even leave the variable as type bool, too). I could go either way.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1101
@@ -1100,3 +1100,3 @@
   Record.push_back(VE.getMetadataOrNullID(N->getRetainedTypes().get()));
-  Record.push_back(VE.getMetadataOrNullID(N->getSubprograms().get()));
+  Record.push_back(/* subprograms */ 0);
   Record.push_back(VE.getMetadataOrNullID(N->getGlobalVariables().get()));
----------------
Not worth changing the record layout to have one fewer records?

================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:460-461
@@ -466,5 +459,4 @@
   // do these waste space, they also can crash gcov.
-  for (Function::iterator BB = F->begin(), E = F->end(); BB != E; ++BB) {
-    for (BasicBlock::iterator I = BB->begin(), IE = BB->end();
-         I != IE; ++I) {
+  for (auto &BB : F) {
+    for (auto &I : BB) {
       // Debug intrinsic locations correspond to the location of the
----------------
Maybe pull this out as a separate patch (& the change to the parameter type from pointer to reference) (& the other range-for-ification later in this loop, etc)

================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:582-583
@@ -591,4 +581,4 @@
       unsigned Edges = 0;
-      for (Function::iterator BB = F->begin(), E = F->end(); BB != E; ++BB) {
-        TerminatorInst *TI = BB->getTerminator();
+      for (auto &BB : F) {
+        TerminatorInst *TI = BB.getTerminator();
         if (isa<ReturnInst>(TI))
----------------
Separate patch?

================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:603-604
@@ -612,4 +602,4 @@
       unsigned Edge = 0;
-      for (Function::iterator BB = F->begin(), E = F->end(); BB != E; ++BB) {
-        TerminatorInst *TI = BB->getTerminator();
+      for (auto &BB : F) {
+        TerminatorInst *TI = BB.getTerminator();
         int Successors = isa<ReturnInst>(TI) ? 1 : TI->getNumSuccessors();
----------------
Separate patch?

================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:181
@@ +180,3 @@
+    auto *NewSP = cast<DISubprogram>(MapMetadata(OldSP, VMap));
+    // There ought to be a better way to do this: ValueMapper will
+    // clone the distinct DICompileUnit. Use the original one instead.
----------------
Maybe make this an explicit "FIXME"? That extra cloning work seems significant, no? (especially if it means cloning all the types in the CU, etc?)


http://reviews.llvm.org/D19034