[PATCH] D87494: Improve LSR debug-info

Wed Sep 23 00:20:09 PDT 2020

markus added a comment.

In D87494#2285378 <https://reviews.llvm.org/D87494#2285378>, @jmorse wrote:

> 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.

I would not expect so. AFAIK it is part of the LLVM debug-info philosophy that no transformation may be affected by the presence of debug-info so that would be a bug (elsewhere) if any transformation behaved differently due to the updated llvm.dbg.value intrinsics. As for that fact that we borrow the SCEV object from LSR to do some additional queries I cant see how that would reasonably affect any further queries passed to that SCEV object. But of course that is just my gut felling.

> 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.

Turns out that it did not. I initially got some compilation errors that led me to beleive that the simple pointer approach was not possible. So I guess this is fine as long as the memory of the DIExpression we are pointing at is not released when it is possiby replaced during `salvageDebugInfo` as part of the normal LSR.

> 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.

Cool. Thanks for taking the time.

================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5794
+        DbgValues.push_back(
+            std::make_tuple(D, DS, D->getExpression()->clone()));
+      }
----------------
jmorse wrote:
> Can this be brace initialized?
Quite possibly but I seem to be unable to pull it off :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87494