[PATCH] D87494: Improve LSR debug-info

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This is looking good, a few nits inline. Some slightly broader questions: do you know if there's any risk of debug-info affecting decisions made by SCEV, i.e. causing codegen to change when compiling -g? I don't have any reason to believe that could be the case, but it's a fear in the back of my mind.

Why does the DIExpression need to become a temporary / be cloned -- my understanding is that it's immutable metadata once created, so we should be able to keep the raw pointer to it.

I ran a clang-3.4 build with this patch, and according to llvm-locstats roughly 6000 variable locations have left the 0% coverage bucket, while roughly 6000 (potentially the same) variables have entered the 100% coverage bucket. That's about 0.2% of all variables, which is a decent improvement to get.



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5794
+        DbgValues.push_back(
+            std::make_tuple(D, DS, D->getExpression()->clone()));
+      }
----------------
Can this be brace initialized?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5831
+      auto &TmpDIExpr = std::get<TempDIExpression>(D);
+      if (isa<UndefValue>(DbgValue->getVariableLocation())) {
+        for (PHINode &Phi : L->getHeader()->phis()) {
----------------
    if (!isa<...>(...))
      continue;`

To save some indentation.



================
Comment at: llvm/test/Transforms/LoopStrengthReduce/dbg-preserve-0.ll:29
+; CHECK-NOT: call void @llvm.dbg.value(metadata i8* undef
+; CHECK: call void @llvm.dbg.value(metadata i8* %lsr.iv, metadata !13, metadata !DIExpression()), !dbg !16
+  store i8 %i.06, i8* %add.ptr, align 1, !dbg !23, !tbaa !24
----------------
It's best practice to capture the metadata reference number and confirm it further down with

  CHECK: ![[VARNUMBER]]  = !DILocalVariable(name: "p"

or similar.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/dbg-preserve-0.ll:39-40
+
+attributes #0 = { nofree norecurse nounwind uwtable writeonly "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone speculatable willreturn }
+
----------------
Best to delete any un-necessary attributes to reduce later maintenence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87494