[PATCH] D16169: [Utils] Fix incorrect dbg.declare store conversion

[Adrian Prantl via llvm-commits]

aprantl added inline comments.

================
Comment at: lib/Transforms/Utils/Local.cpp:1138
@@ +1137,3 @@
+        User *U = AIUse.getUser();
+        if (StoreInst *SI = dyn_cast<StoreInst>(U)) {
+          if (AIUse.getOperandNo() == 1)
----------------
loladiro wrote:
> loladiro wrote:
> > aprantl wrote:
> > > Does turning this if cascade into a switch() make it any more readable?
> > I think that would introduce a lot of extra casting, which would probably make it less readable (assuming you were thinking of a cast on ->getOperand()), if there's a better syntax, do let me know.
> Sorry ->getOpcode of course.
Yes, that's what I meant. If it doesn't improve the readability (speed doesn't matter with three cases) let's leave it as is.

================
Comment at: test/Transforms/Util/store-first-op.ll:13
@@ +12,3 @@
+; is correct.
+; CHECK-NOT: @llvm.dbg.value(metadata %foo* %getU
+  call void @llvm.dbg.declare(metadata %foo* %getU, metadata !3, metadata !6), !dbg !7
----------------
loladiro wrote:
> aprantl wrote:
> > I would still add a positive check — this way we'll be notified when the testcase needs to be updated.
> What kind of positive test are you imagining. In the corrected version, no dbg.* intrinsic is inserted.
My thought was that if no dbg.value is inserted the dbg.declare should still be there but that isn't actually the case. In this testcase the alloca is dead because there is no store to it or load, so not having any intrinsics makes sense.

Hm. This brings up an interesting point: Shouldn't LowerDbgDeclare() replace the alloca in the dbg.declare instruction with an %undef or alternatively leave the dbg.declare in there if the dbg.declare was not lowered? Otherwise we're going to loose function arguments and local variables.


Repository:
  rL LLVM

http://reviews.llvm.org/D16169