[PATCH] D99273: [DebugInfo] Support for signed constants inside DIExpression

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3052
       // Emit constant global variables in a global symbol section.
-      if (GlobalMap.count(GVE) == 0 && DIE->isConstant()) {
+      if (GlobalMap.count(GVE) == 0 && DIE->isUnsignedConstant()) {
         CVGlobalVariable CVGV = {DIGV, DIE};
----------------
SouraVX wrote:
> dstenb wrote:
> > Why do we only care about unsigned constants here?
> > 
> > (And in the other cases where `isConstant()` is changed to `isUnsignedConstant()`)
> Thanks for the question! As noted in the patch summary, we are trying to be conservative here.
> Motivation behind this change, Previous code made a check for `isConstant` which sounds/seems ambiguous to me when I introduced `isSignedConstant` so I took the opportunity to make the change conservatively.
Okay, thanks! So all of those changes are done conservatively, rather than signed constants being unwanted there. Can we add TODO comments that say that we should investigate if signed constants should be covered also for each of these cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99273