[PATCH] D69809: Wrong debug info generated at -O2 (-O0 is correct)

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

A few nits inline, otherwise great!



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3225
+
       eraseInstFromFunction(*I);
       ++NumDeadInst;
----------------
kamleshbhalui wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > What about other users of `eraseInstFromFunction()`?
> > > Should this go into that function?
> > My comment wasn't addressed.
> I have seen this is in EarlyCSE.
> And yes, It should go into several other places too, But need to come up with testcases.
replaceDbgUsesWithUndef should quite possibly go into the intstruction-deleting methods, D69787 even puts salvageDebugInfo in those methods too. It's not clear to me why this hasn't always been the case (performance)? IMO, that would be another patch though.


================
Comment at: llvm/test/Transforms/InstCombine/debuginfo-dce.ll:36-37
 ; CHECK: define void @salvage_load
 ; CHECK-NEXT: entry:
-; CHECK-NOT: dbg.value
+; CHECK-NEXT: dbg.value
   store %struct.entry* %1, %struct.entry** %im_not_dead, align 8
----------------
kamleshbhalui wrote:
> davide wrote:
> > Do you know why this changed?
> Before the fix instcombiner was deleting all the users (even dbg.value ) of a trivially dead instruction.
> But now we mark dbg.value as undef instead of deleting it .
> and that's why modified the check like this.
> 
> 
Best to give the checked-for dbg.value an operand, so "dbg.value(metadata %struct.entry *undef" or similar. Otherwise this isn't fully checking for the correct behaviour here.


================
Comment at: llvm/test/Transforms/InstCombine/pr43893.ll:1
+; RUN: opt -instcombine -S -o - %s | FileCheck %s
+
----------------
It'd be nice to have a summary comment at the top of the test, explaining what it's testing for.


================
Comment at: llvm/test/Transforms/InstCombine/pr43893.ll:32
+attributes #0 = { noinline nounwind uwtable }
+attributes #1 = { nounwind readnone speculatable willreturn }
+
----------------
We usually delete all attributes from tests, as they're an un-necessary maintenance burden.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69809