[PATCH] D23715: Add @llvm.dbg.value entries for the phi node created by -mem2reg

[Keith Walker via llvm-commits]

keith.walker.arm added a comment.

Thanks for the review, I will update the patch taking into account of your comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1066-1078
@@ +1065,15 @@
+  // not inserting the same dbg.value intrinsic over and over.
+  llvm::BasicBlock::InstListType::iterator NextI(I);
+  while (NextI != I->getParent()->getInstList().end()) {
+    ++NextI;
+    if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(NextI)) {
+      if (DVI->getValue() == I &&
+          DVI->getOffset() == 0 &&
+          DVI->getVariable() == DIVar &&
+          DVI->getExpression() == DIExpr)
+        return true;
+    } else
+      return false;
+  }
+  return false;
+}
----------------
dblaikie wrote:
> This might be better written as an llvm::any_of (or at least llvm::find_if), perhaps?
I'll try doing as you suggest as see if I can re-write it in a more concise form.

================
Comment at: lib/Transforms/Utils/Local.cpp:1067-1069
@@ +1066,5 @@
+  llvm::BasicBlock::InstListType::iterator NextI(I);
+  while (NextI != I->getParent()->getInstList().end()) {
+    ++NextI;
+    if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(NextI)) {
+      if (DVI->getValue() == I &&
----------------
dblaikie wrote:
> This looks like it might try to dyn_cast the end iterator, which I would imagine isn't valid in general? (is there a reason it's incrementing first then doing work, rather than a more normal for (even range-for) loop?)
The main reason is because I originally copied it from the previous function and then made it work forward rather than back and also become a loop - so it's basic structure is similar to the previous function.    I will re-work it.

================
Comment at: test/CodeGen/AArch64/phi-dbg.ll:1-4
@@ +1,5 @@
+; RUN: llc -O0 %s -mtriple=aarch64 -o - | FileCheck %s
+
+; Test that a DEBUG_VALUE node is create for variable c after the phi has been
+; converted to a ldr.    The DEBUG_VALUE must be *after* the ldr and not before it.
+
----------------
dblaikie wrote:
> Seems like this patch could be split in two - this test and the associated improvements to lib/CodeGen - and, separately, the changes mem2reg (& the other test) mentioned in the description.
Good idea.  I will split it into 2 separate patches.


https://reviews.llvm.org/D23715