[PATCH] D88896: [DebugInfo][InstrRef][3/4] Produce DBG_INSTR_REFs for all* variable locations

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineFunction.h:510
+  ///          value, or None if nothing could be recovered.
+  Optional<DebugInstrOperandPair> salvageCopySSA(MachineInstr &MI);
+
----------------
Orlando wrote:
> Does this need to return an `Optional`? As far as I can tell `salvageCopySSA` never returns `None`, and the only callsite doesn't check whether the returned result has a value or not.
/me scratches head. I guess I prototyped this with it returning an optional, then revised it to assert if any value wasn't available.

I'll update this to not return an optional.


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:1039-1040
+      Offset = TRI.getSubRegIdxOffset(SubReg);
+    if (OldRegBits >= NewRegBits || Offset)
+      SubReg = 0;
+
----------------
Orlando wrote:
> I'm struggling to follow this part. When you say "only remember truncations or offsets" am I right in thinking that setting `SubReg = 0` causes some kind of mismatch that is picked up and handled by LiveDebugValues/InstrRefBasedImpl in D88894?
Probably because it's a cop-out -- if I compile this:

    void ext(int b);
    int foo(char a)
    {
        ext(a);
        return 0;
    }

I get `DBG_VALUE $edi` for `b` rather than `DBG_VALUE $al`, which has encouraged me to be fairly lax about accurate widening for parameters. Off the top of my head, I can't think of any reason to omit the subregister information, I'll run some experiments to see what effect it has.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88896/new/

https://reviews.llvm.org/D88896