[PATCH] D30919: Salvage debug info from instructions about to be deleted

[David Majnemer via Phabricator via llvm-commits]

majnemer added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1366-1375
+  if (auto *BitCast = dyn_cast<BitCastInst>(&I)) {
+    // Bitcasts are entirely irrelevant for debug info. Rewrite the dbg.value to
+    // use the cast's source.
+    findDbgValues(DbgValues, &I);
+    for (auto *DVI : DbgValues) {
+      DVI->setOperand(0, MDWrap(I.getOperand(0)));
+      DEBUG(dbgs() << "SALVAGE: " << *DVI << '\n');
----------------
Any idea on the perf impact of this change?


================
Comment at: lib/Transforms/Utils/Local.cpp:1386
+        DIBuilder DIB(M, /*AllowUnresolved*/ false);
+        DIExpr = BuildReplacementDIExpr(DIB, DIExpr, /*Deref*/ false,
+                                        Offset.getLimitedValue());
----------------
majnemer wrote:
> dblaikie wrote:
> > Possible refactoring: using an enum rather than a bool parameter, to help legibility.
> Hmm, should this be isSignedIntN and getSExtValue? I could believe it shouldn't be but I'd like to understand the rationale.
BuildReplacementDIExpr takes an int for the offset. Won't this be problematic if isIntN returns true but getLimitedValue returns something bigger than INT_MAX?


================
Comment at: lib/Transforms/Utils/Local.cpp:1386-1387
+        DIBuilder DIB(M, /*AllowUnresolved*/ false);
+        DIExpr = BuildReplacementDIExpr(DIB, DIExpr, /*Deref*/ false,
+                                        Offset.getLimitedValue());
+        DVI->setOperand(0, MDWrap(I.getOperand(0)));
----------------
dblaikie wrote:
> Possible refactoring: using an enum rather than a bool parameter, to help legibility.
Hmm, should this be isSignedIntN and getSExtValue? I could believe it shouldn't be but I'd like to understand the rationale.


================
Comment at: lib/Transforms/Utils/Local.cpp:1394
+  } else if (auto *Load = dyn_cast<LoadInst>(&I)) {
+    SmallVector<DbgValueInst *, 1> DbgValues;
+    findDbgValues(DbgValues, &I);
----------------
Doesn't this shadow the other DbgValues?


https://reviews.llvm.org/D30919