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

Adrian Prantl via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 14 16:55:58 PDT 2017


aprantl marked 4 inline comments as done.
aprantl added a comment.

New version attached.



================
Comment at: lib/Transforms/Utils/Local.cpp:1382-1383
+      if (GEP->accumulateConstantOffset(M.getDataLayout(), Offset)) {
+        if (!Offset.isIntN(64))
+          return;
+        auto *DIExpr = DVI->getExpression(); 
----------------
dblaikie wrote:
> Is this tested?
No and apparently this is sort of redundant. While attempting to create  a testcase for this I learned that it is impossible to create GEP offsets with a type other than i32. I should probably just remove it.


================
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)));
----------------
majnemer wrote:
> 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?
It looks like it doesn't matter. In the testcase the offset is -8 and it comes out correctly. Not sure yet whether this is a happy accident or the documed behavior.


https://reviews.llvm.org/D30919





More information about the llvm-commits mailing list