[PATCH] D138995: [CodeView] Add support for local S_CONSTANT records

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

Thanks, that's a nice improvement! Out of curiosity does this optimization happen at `-O0` too? (when using clang-cl)
Does MSVC generate `S_CONSTANT` as well for your test-case?



================
Comment at: lld/test/COFF/Inputs/pdb-local-constants.s:191
+	.long	0x0                             # Id
+	.asciz	"\"-cc1\" \"-ferror-limit\" \"19\" \"-fcolor-diagnostics\" \"-mllvm\" \"-treat-scalable-fixed-error-as-warning\" \"-disable-free\" \"-S\" \"-x\" \"c++\" \"-target-cpu\" \"x86-64\" \"-tune-cpu\" \"generic\" \"-triple\" \"x86_64-unknown-windows-msvc19.20.0\" \"-resource-dir\" \"/data/code/llvm-project/build/lib/clang/16\" \"-isystem\" \"/data/code/llvm-project/build/lib/clang/16/include\" \"-std=c++14\" \"-fcxx-exceptions\" \"-fexceptions\" \"-fmath-errno\" \"-fms-compatibility\" \"-fdelayed-template-parsing\" \"-pic-level\" \"2\" \"-fdeprecated-macro\" \"-fms-compatibility-version=19.20.0\" \"-ffp-contract=on\" \"-fno-experimental-relative-c++-abi-vtables\" \"-fno-file-reproducible\" \"-O2\" \"-fdebug-compilation-dir=/home/tobias/code/llvm-project/build\" \"-fcoverage-compilation-dir=/home/tobias/code/llvm-project/build\" \"-faddrsig\" \"-fno-use-cxa-atexit\" \"-gcodeview\" \"-gno-column-info\" \"-funwind-tables=2\" \"-mconstructor-aliases\" \"-vectorize-loops\" \"-vectorize-slp\" \"-clear-ast-before-backend\" \"-finline-functions\" \"-debug-info-kind=constructor\" \"-fno-emulated-tls\" \"-fdiagnostics-hotness-threshold=0\" \"-fdiagnostics-misexpect-tolerance=0\"" # StringData
+	# BuildInfo (0x1009)
----------------
Since we have `-gno-codeview-command-line` can you leave out this part? It adds unnecessary noise which is not required for this test.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1347
+    {
+      // When we don't have a location this is usually becasue LLVM has transformed
+      // it into a constant and we only have a llvm.dbg.value. Since we can't
----------------
s/becasue/because/


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1349
+      // it into a constant and we only have a llvm.dbg.value. Since we can't
+      // represent these well in CodeView since it only works on regs and variables
+      // we will pretend this to be a constant value to at least have it show up
----------------
'since' repeated twice, not sure if this could be rephrased a bit?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1355
+        APSInt Val(APInt(64, Op.getImm()), false);
+        Var.ConstantValue = Val;
+      }
----------------
nit: could also be `Val.ConstantValue = APSInt(APInt(64, Op.getImm()), false);`


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2800
   for (const LocalVariable *L : Params)
-    emitLocalVariable(FI, *L);
+      emitLocalVariable(FI, *L);
 
----------------
unwanted indent?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2803
   // Next emit all non-parameters in the order that we found them.
   for (const LocalVariable &L : Locals)
+    if (!L.DIVar->isParameter()) {
----------------
I would add the extra braces at the end of the `for` line, to enclose the scope below.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2808
+        // instead of a S_LOCAL in order to be able to represent it
+        // at all.
+        const DIType *Ty = L.DIVar->getType();
----------------
Could this comment fit on the line above? You can also write the entire comment on one line, and then `git clang-format` reformat it.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h:107
     bool UseReferenceType = false;
+    llvm::Optional<APSInt> ConstantValue;
   };
----------------
I think the way to go forward is to use `std::optional`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138995