[PATCH] D114565: [InstrProf] Attach debug info to counters

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:273
 
-  if (!DataSize)
+  if (!CountersSize)
     return 0;
----------------
kyulee wrote:
> Is it no diff change? I wonder if there is a case where `CountersSize` is 0 but not `DataSize`  from a value probe.
> If not, should we make a separate change?
> I wonder if there is a case where CountersSize is 0 but not DataSize from a value probe.
Ah, yes I believe this is possible. I'll fix this here.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:851
+      addString(AnnotationDie, dwarf::DW_AT_const_value, Value->getString());
+    else if (const auto *Expr = dyn_cast<DIExpression>(ValueOp))
+      addConstantValue(
----------------
kyulee wrote:
> It checks an expression but appears to apply the constant case below. Should it check a constant expression, instead?
Oh we actually don't need to use `DIExpression` at all. Instead we can emit `ConstantAsMetadata` to simplify it quite a bit.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:647
+  PD.NumValueSites[ValueKind] =
+      std::max(PD.NumValueSites[ValueKind], (uint32_t)(Index + 1));
 }
----------------
kyulee wrote:
> It appears this is a NFC. Should we make a separate change?
Yeah I can move this out of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114565