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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2477-2480
+  if (Value.isConstantInt()) {
+    DwarfExpr.addUnsignedConstant(Value.getConstantInt()->getValue(), AP);
+    return;
+  }
----------------
What's this change doing? It looks new & not sure what the old code would've been doing previously under this condition (isConstantInt) 


================
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());
----------------
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)


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:233
     addUnsignedConstant(*Data++);
+    addStackValue();
     if (Offset == 0 && Size <= 64)
----------------
Is this a separate bugfix? Could go in a separate patch/test/etc?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90916