[PATCH] D90916: [DebugInfo] Emit locations for large constant integers

[Pavel Labath via Phabricator via llvm-commits]

labath marked 4 inline comments as done.
labath added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:202
 
-void DwarfExpression::addUnsignedConstant(const APInt &Value) {
+void DwarfExpression::addUnsignedConstant(APInt Value, const AsmPrinter &AP) {
   assert(isImplicitLocation() || isUnknownLocation());
----------------
labath wrote:
> dblaikie wrote:
> > This change looks like it'd have two effects:
> > 
> > 1) existing callers of addUnsignedConstant now might get DW_OP_implicit_values where they didn't before
> > 2) the new call from addConstantFP might lead to that use case getting DW_OP_const values it didn't get before
> > 
> > Could those two situations be separated into distinct patches with distinct tests? (I mean, could be the same test file, but making it clear how adding this new functionality to addUnsignedConstant impacts existing callers, then separately how changing addConstantFP to use addUnsignedConstant changes that)
> I elaborate on (1) below. You're right about (2). I did check that gdb&lldb can actually handle the expanded usage of DW_OP_const, but I was too lazy to turn it into a test. I'll try to split that off into a separate patch.
Created a separate refactoring patch here: https://reviews.llvm.org/D91058.

On second though, I don't think there's anything additional to test (until we support long double constants that is). The function was only being called for values smaller than 64 bits, and the existing implict_value FP tests have an SCE test which confirms that DW_OP_stack value is emitted in the same way as before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90916