[PATCH] D49454: [DebugInfo] LowerDbgDeclare: Add derefs when handling CallInst users

[Shafik Yaghmour via Phabricator via llvm-commits]

shafik added inline comments.


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1253
   SDDbgValue *getFrameIndexDbgValue(DIVariable *Var, DIExpression *Expr,
-                                    unsigned FI, const DebugLoc &DL,
-                                    unsigned O);
+                                    unsigned FI, bool IsIndirect,
+                                    const DebugLoc &DL, unsigned O);
----------------
Instead of using a `bool` for `IsIndirect` perhaps using an `enum` with two enumerator `Direct` and `Indirect` would be more readable and maintainable?

I make this comment since I noticed you were commenting your usage below, which indicated lack of clarity.


================
Comment at: lib/CodeGen/SelectionDAG/InstrEmitter.cpp:700
     // EmitTargetCodeForFrameDebugValue is responsible for allocation.
-    return BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE))
-        .addFrameIndex(SD->getFrameIx())
-        .addImm(0)
-        .addMetadata(Var)
-        .addMetadata(Expr);
+    auto FrameMI = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE))
+                       .addFrameIndex(SD->getFrameIx());
----------------
Does using `auto` gain us much here?


================
Comment at: lib/IR/DebugInfoMetadata.cpp:855
+      NewOps.append(Ops.begin(), Ops.end());
+      Ops = None;
+    }
----------------
Are we expecting to only do this once and that is why we don't clear `NewOps` after the `appendToVector` below?


https://reviews.llvm.org/D49454