[LICM] Salvage DebugInfo from dying Instructions

[gramanas]

Summary

LICM deletes trivially dead instructions which it won't attempt to sink.
Attempt to salvage debug values which reference these instructions.

This is similar to rL325660, which did the same thing in the BDCE pass.

[vsk]

Thanks for working on this! The change in LICM looks good, but the tests need a little bit of work.

test/Transforms/LICM/2011-04-09-RAUW-AST.ll

2 ↗

(On Diff #137928)

Why does this test need to be updated in addition to sinking.ll? It seems like one test case should be enough to exercise the new functionality.

test/Transforms/LICM/sinking.ll

316

Generally we try not to hardcode metadata numbers into CHECK lines, because it can make for a brittle test case. We wouldn't want this test to fail if e.g the order in which metadata are emitted changes.

Instead of "!143", writing "{{.*}}" here would be fine.

317

This used to be a CHECK-NEXT, but now it's a CHECK. That loosens the test, as FileCheck will no longer require that the "ret i32" occur immediately after the "trunc i64" instruction.

Can you move the CHECK-NEXT back to where it was, and add the DEBUGIFY check line after it?

[vsk]

test/Transforms/LICM/sinking.ll

316

When testing this patch out locally, I found that only dbg.value salvaged in sinking.ll is in the function @test11. In that function, there's a dead getelementptr instruction which LICM deletes. Right before it goes away, the call you added to salvageDebugInfo() rewrites the effect of the GEP as a DIExpression:

call void @llvm.dbg.value(metadata %Ty* @X2, metadata !127, metadata !DIExpression(DW_OP_stack_value))

Could you remove the changes to the @PR18753 function, and add checks to @test11 instead?

[gramanas]

• fix redundant debugify test

[vsk]

LGTM. Is it all right if I commit this for you, or would you like to set up an account and commit it yourself?

[vsk]

Ah, I think there's still a problem with the test.

test/Transforms/LICM/sinking.ll

254

The label to check for should be @test11

255

The dbg.value should look like: call void @llvm.dbg.value(metadata %Ty* @X2, metadata {{.*}}, metadata !DIExpression(DW_OP_stack_value))

If you run "check-llvm" after making this change, the tests should all pass.

[gramanas]

• Fix test label

[vsk]

This landed as r327800. @gramanas, if you add "Differential Revision: <URL to the review>" to the last line of your commit message, Phabricator will automatically close the review for you when you commit.

lib/Transforms/Scalar/LICM.cpp

for (DomTreeNode *DTN : reverse(Worklist)) {
 
     for (BasicBlock::iterator II = BB->end(); II != BB->begin();) {
       Instruction &I = *--II;
 
       // If the instruction is dead, we would try to sink it because it isn't
       // used in the loop, instead, just delete it.
       if (isInstructionTriviallyDead(&I, TLI)) {
         DEBUG(dbgs() << "LICM deleting dead inst: " << I << '\n');
+        salvageDebugInfo(I);
         ++II;
         CurAST->deleteValue(&I);
         I.eraseFromParent();
         Changed = true;
         continue;
       }
 
       // Check to see if we can sink this instruction to the exit blocks

test/Transforms/LICM/sinking.ll

 ; RUN: opt < %s -basicaa -licm -S | FileCheck %s
+; RUN: opt < %s -debugify -basicaa -licm -S | FileCheck %s -check-prefix=DEBUGIFY
 
 declare i32 @strlen(i8*) readonly nounwind
 
 declare void @foo()
 
 ; Sink readonly function.
 define i32 @test1(i8* %P) {
   br label %Loop

 Loop:
   %dead = getelementptr %Ty, %Ty* @X2, i64 0, i32 0
   br i1 false, label %Loop, label %Out
 Out:
   ret void
 ; CHECK-LABEL: @test11(
 ; CHECK:     Out:
 ; CHECK-NEXT:  ret void
+
+; DEBUGIFY-LABEL: @test11(
+; DEBUGIFY: call void @llvm.dbg.value(metadata %Ty* @X2, metadata {{.*}}, metadata !DIExpression(DW_OP_stack_value))
 }
 
 @c = common global [1 x i32] zeroinitializer, align 4
 
 ; Test a *many* way nested loop with multiple exit blocks both of which exit
 ; multiple loop nests. This exercises LCSSA corner cases.
 define i32 @PR18753(i1* %a, i1* %b, i1* %c, i1* %d) {
 entry:
l1.latch:
   br label %l1.header
 
 exit:
   %lcssa = phi i32 [ %l, %l4.latch ], [ %l, %l4.body ]
 ; CHECK-LABEL: @PR18753(
 ; CHECK:       exit:
 ; CHECK-NEXT:    %[[LCSSAPHI:.*]] = phi i64 [ %iv, %l4.latch ], [ %iv, %l4.body ]
 ; CHECK-NEXT:    %l.le = trunc i64 %[[LCSSAPHI]] to i32
 ; CHECK-NEXT:    ret i32 %l.le
 
   ret i32 %lcssa
 }
 
 ; Can't sink stores out of exit blocks containing indirectbr instructions
 ; because loop simplify does not create dedicated exits for such blocks. Test
 ; that by sinking the store from lab21 to lab22, but not further.
 define void @test12() {
 ; CHECK-LABEL: @test12