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

[Vedant Kumar via Phabricator via llvm-commits]

vsk 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);
----------------
shafik wrote:
> 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.
I think commented use of a bool is reasonably clear. Refactoring the code to use enums would be fine as a follow-up change, but doesn't seem necessary for this patch.


================
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());
----------------
shafik wrote:
> Does using `auto` gain us much here?
Yes, it's the convention in the backends to feign ignorance as to what BuildMI returns:
```
$ gg -E "auto [^=]+= BuildMI" | wc -l
      52
```
It's a bit distracting to see 'MachineInstrBuilder' here, c.f unordered_map iterators.


================
Comment at: lib/IR/DebugInfoMetadata.cpp:855
+      NewOps.append(Ops.begin(), Ops.end());
+      Ops = None;
+    }
----------------
shafik wrote:
> Are we expecting to only do this once and that is why we don't clear `NewOps` after the `appendToVector` below?
Right. I'll try to make that clearer in the final comment in this function.


https://reviews.llvm.org/D49454