[PATCH] D61933: [DebugInfo] Update loop metadata for inlined loops

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Seems reasonable, nitpicks inside.



================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1378
+  for (unsigned i = 1; i < OrigLoopId->getNumOperands(); ++i) {
+    if (MDNode *MD = dyn_cast<MDNode>(OrigLoopId->getOperand(i))) {
+      // Update the DILocations to encode the inlined-at metadata.
----------------
what can this be except an MDNode?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1380
+      // Update the DILocations to encode the inlined-at metadata.
+      if (DILocation *DL = dyn_cast<DILocation>(MD))
+        MDs.push_back(inlineDebugLoc(DL, InlinedAt, Ctx, IANodes));
----------------
what can this be except a DILocation?


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1381
+      if (DILocation *DL = dyn_cast<DILocation>(MD))
+        MDs.push_back(inlineDebugLoc(DL, InlinedAt, Ctx, IANodes));
+      else
----------------
this is reusing the same code used for inlining instructions — looks good


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1388
+  MDNode *NewLoopID = MDNode::getDistinct(Ctx, MDs);
+  // Insert the self-referencial LoopID.
+  NewLoopID->replaceOperandWith(0, NewLoopID);
----------------
`referential`


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1420
       if (DebugLoc DL = BI->getDebugLoc()) {
-        auto IA = DebugLoc::appendInlinedAt(DL, InlinedAtNode, BI->getContext(),
-                                            IANodes);
-        auto IDL = DebugLoc::get(DL.getLine(), DL.getCol(), DL.getScope(), IA);
+        IDL = inlineDebugLoc(DL, InlinedAtNode, BI->getContext(), IANodes);
         BI->setDebugLoc(IDL);
----------------
this is just factored out — LGTM


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1432
+      if (IDL)
+        continue;
+
----------------
this is weird, why no move the loop metadata update *above* the DILocation update?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61933