[PATCH] D30190: [LoopRotate] Update dbg.value calls

[Adrian Prantl via Phabricator via llvm-commits]

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

Seems generally plausible. LGTM with all inline comments addressed.



================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:178
 
+static void InsertDebugValues(BasicBlock *OrigHeader,
+                              SmallVectorImpl<PHINode*> &InsertedPHIs) {
----------------
Technically LLVM coding guidelines want this to start with a lower case character. (but I see that other functions in the same file also violate this...)


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:178
 
+static void InsertDebugValues(BasicBlock *OrigHeader,
+                              SmallVectorImpl<PHINode*> &InsertedPHIs) {
----------------
aprantl wrote:
> Technically LLVM coding guidelines want this to start with a lower case character. (but I see that other functions in the same file also violate this...)
Comment what the function is doing?


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:186
+      if (isa<PHINode>(DbgII->getVariableLocation()))
+        DbgValueMap.insert(std::make_pair(DbgII->getVariableLocation(), DbgII));
+    }
----------------
`DbgValueMap.insert({DbgII->getVariableLocation(), DbgII)});`


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:191
+  // Then iterate through the new PHIs and look to see if they use one of the
+  // previously mapped PHIs. If so, insert a new dbg.value call that will
+  // propagate the info through the new PHI.
----------------
s/call/instrinsic/


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:198
+      if (V != DbgValueMap.end()) {
+        DbgInfoIntrinsic *DbgII = dyn_cast<DbgInfoIntrinsic>(V->second);
+        Instruction *NewDbgII = DbgII->clone();
----------------
1. auto
2. Why do you need a dyn_cast here? I would have either expected a static_cast, or something that actually checks the result.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:389
+
+  // Attach dbg.value calls to the new phis if that phi uses a value that
+  // previously had debug metadata attached. This keeps the debug info
----------------
call -> intrinsic


================
Comment at: test/Transforms/LoopRotate/phi-dbgvalue.ll:2
+; RUN: opt -S -loop-rotate < %s | FileCheck %s
+
+;CHECK-LABEL: func
----------------
Comment what the test is testing.


https://reviews.llvm.org/D30190