[PATCH] D65368: [DebugInfo] LiveDebugValues: Don't drop fragment information when restoring spills

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

> This could be fixed, possibly easily, by adding a DIExpression field to VarLoc and using that expression on restore. I've shied away from this as it could reduce performance, and the risk that adding new features/facilities won't be acceptable to merge into the LLVM-9 branch to fix the upcoming release. (I've no experience with what is or isn't seen as acceptable).

As a general point, we should avoid discussing the merit of a patch based on LLVM's release schedule. If we are concerned about this patch it's best to split it into two parts, an uncontroversial one that is an obvious improvement that then can get cherry-picked without being afraid and a follow-up patch that does things right (but at potentially a performance cost that may need to be evaluated). But as far as trunk is concerned, our priority should always be what's right in the long term.

> This update demonstrates one of the alternatives -- pointing the VarLoc object's DBG_VALUE at the unspilt DBG_VALUE allows the spill-restore code to access the correct expression upon restore. This requires slightly more logic when propagating locations, as we can't just blindly clone DBG_VALUEs any more.

I like the new patch! It looks like you discovered a few more pre-existing bugs though, so I'd suggest to do the following:

1. Create a patch that disables spill-recognition for complex parameters, cherry-pick it to the release. Should be obviously correct.
2. Create a patch that implements *restoring* of complex expressions (e.g., by pointing to the unspilt DBG_VALUE) and potentially cherry-pick. (Should also be obviously correct.)
3. Create a patch to enable describing the spilt location of complex expressions and iterate until all bugs are fixed (possibly lands after the release).

Let me know what you think!



================
Comment at: lib/CodeGen/LiveDebugValues.cpp:694
+        *MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(), false, NewReg,
+        DebugInstr->getDebugVariable(), DebugInstr->getDebugExpression());
     VarLoc VL(*NewDebugInstr, LS);
----------------
So here we are restoring the original expression since DebugInstr points to the pre-spill DBG_VALUE.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:1121
+        Reg = TRI->getFrameRegister(*DebugInstr->getMF());
+        IsIndirect = true;
+      }
----------------
Out of curiosity: What happens if IsIndirect was already true? Are we loosing one level of indirection or does this work out?


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

https://reviews.llvm.org/D65368