[deadargelim] Attach dbg info to the insert/extractvalue instructions

[djtodoro]

Summary

The bug was found by the LLVM DI Checker (pushed as -debugify=original mode) and this was used in the RFC [0] for the utility.

Attach DbgLoc on insertvalue/extractvalue instructions created by DeadArgumentElimination.
This fixes the PR46350.

[0] https://groups.google.com/forum/#!msg/llvm-dev/QOyF-38YPlE/G213uiuwCAAJ

[vsk]

Hi @djtodoro, this popped up in my mail filter so I thought I'd drop a line :). It sounds like there's a substantial amount of overlap with the debugify stuff and the tool here (https://github.com/djolertrk/llvm-di-checker). Do we need both?

For the issue found here for example, one might do: ./bin/llvm-lit -Dopt="opt -debugify-each" test/Transforms/DeadArgElim -av 2>&1 | less, and this prints:

PASS: LLVM :: Transforms/DeadArgElim/multdeadretval.ll (28 of 34)
Script:
--
: 'RUN: at line 5';   opt -debugify-each < /Users/vsk/src/llvm-project-master/llvm/test/Transforms/DeadArgElim/multdeadretval.ll -deadargelim -instcombine -dce -S | /Users/vsk/src/builds/llvm-project-master-RA/bin/not grep i16
--
Exit Code: 0

Command Output (stderr):
--
ERROR: Instruction with empty DebugLoc in function test --  %oldret = extractvalue { i16, i32 } %B, 1
ERROR: Instruction with empty DebugLoc in function test2 --  %oldret = extractvalue { i32, i16 } %B, 0
ERROR: Instruction with empty DebugLoc in function test3 --  %oldret = insertvalue { i16, i32 } undef, i32 %ret, 1
ERROR: Instruction with empty DebugLoc in function test5 --  %oldret = extractvalue { i32, i32, i16 } %C, 0
...

I think there are advantages to looking at real code outside of the llvm unit/regression tests, just wondering whether we need to redo all the DI checking / stats collection work.

[djtodoro]

In D81939#2096463, @vsk wrote:

Hi @djtodoro, this popped up in my mail filter so I thought I'd drop a line :). It sounds like there's a substantial amount of overlap with the debugify stuff and the tool here (https://github.com/djolertrk/llvm-di-checker). Do we need both?

For the issue found here for example, one might do: ./bin/llvm-lit -Dopt="opt -debugify-each" test/Transforms/DeadArgElim -av 2>&1 | less, and this prints:

PASS: LLVM :: Transforms/DeadArgElim/multdeadretval.ll (28 of 34)
Script:
--
: 'RUN: at line 5';   opt -debugify-each < /Users/vsk/src/llvm-project-master/llvm/test/Transforms/DeadArgElim/multdeadretval.ll -deadargelim -instcombine -dce -S | /Users/vsk/src/builds/llvm-project-master-RA/bin/not grep i16
--
Exit Code: 0

Command Output (stderr):
--
ERROR: Instruction with empty DebugLoc in function test --  %oldret = extractvalue { i16, i32 } %B, 1
ERROR: Instruction with empty DebugLoc in function test2 --  %oldret = extractvalue { i32, i16 } %B, 0
ERROR: Instruction with empty DebugLoc in function test3 --  %oldret = insertvalue { i16, i32 } undef, i32 %ret, 1
ERROR: Instruction with empty DebugLoc in function test5 --  %oldret = extractvalue { i32, i32, i16 } %C, 0
...

I think there are advantages to looking at real code outside of the llvm unit/regression tests, just wondering whether we need to redo all the DI checking / stats collection work.

Hi @vsk, thanks for the comment! :)

The debugify tool is very useful for the regression testing/Pass verification; and I use it that way and I think we can all use it in the combination with this. The idea here was to introduce a tool (let me say a sibling of the debugify) that will deal with real debug info metadata. I know that debugify already deals with the DILocations, but I thought to introduce support for all kind of metadata (including DIGlobalVariables, DILexicalBlocks, etc.; which is hard to maintain as synthetic debug info), and support DILocations along the way as well. In addition, I think that extending this to MIR level is also doable/may be straight forward.

We all use the compiler on real projects, and users report issues regarding debug info saying "My variable 'a' is optimized out, but it should not be" or "I cannot attach breakpoint to function 'f' (or instruction 'i')", so I found this tool very useful in these situations, since it points (or it will point when improved; this is initial stage) to real spot of the bug that caused the issue for the concrete entity. Also, there are companies with downstream passes and frontend features that may not be direct cause of the bug, but the bug can occur somewhere else in the pipeline as a consequence. So, I think (by using this) we can come up with variety of test cases that may not be present in the existing tests.

I'll send an RFC and please comment on that! Thanks again!

[djtodoro]

• Cover all the instructions
• Cover all the cases with the test
• Remove [NOT FOR COMMIT] part

[aprantl]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

973

@vsk: would it be better style to ad-hoc create an IRBuilder with the correct debug location here?

llvm/test/Transforms/DeadArgElim/multdeadretval.ll

11

I think it would be much better to have a targeted check here that checks the actual instructions. Debugify is great for finding bugs, but I wouldn't recommend it for writing testcases. In this form, nothing guarantees that the code path that triggers the bugfix is still exercised by the test if the pass is modified and the test quickly looses its usefulness.

[djtodoro]

• Check the actual instructions instead of final debugify result in the test provided

[djtodoro]

llvm/test/Transforms/DeadArgElim/multdeadretval.ll

11

I've just addressed this.

[aprantl]

llvm/test/Transforms/DeadArgElim/multdeadretval.ll

12

Thanks!

Should we also check that the !dbg attachment is the same as some other instruction? Or are we really fine with just *any* !dbg location?

[djtodoro]

llvm/test/Transforms/DeadArgElim/multdeadretval.ll

12

It makes sense to me to test the actual locations. I've addressed that as well. Thanks you @aprantl!

[djtodoro]

• Check actual locations in the test

[vsk]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

973

Yes, that seems to be the common idiom. I recommend using IRBuilder<NoFolder> to avoid spurious test changes.

llvm/test/Transforms/DeadArgElim/multdeadretval.ll

7

I recommend copying this test, modifying it to include debug info, and dropping the -enable-debugify=synthetic part. This bugfix doesn't need to depend on the debugify original mode patchset. Also, the hardcoded checks for DILocation line numbers will make this test hard to modify, so if we want to check specific synthetic line numbers I think we'd be better served by a dedicated test.

[djtodoro]

llvm/test/Transforms/DeadArgElim/multdeadretval.ll

7

It makes sense to me! Thanks!

[djtodoro]

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

973

Oh.. The IRBuilder<> will generate a debug loc by default (via insert() method).

[djtodoro]

• Use IRBuilder<> since it generates dbg loc by default
• Create new test

[aprantl]

lgtm from my point of view now.

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

981–982

Nice!

llvm/test/DebugInfo/X86/dbgloc-insert-extract-val-instrs.ll

5 ↗

(On Diff #275695)

Why use a custom prefix if there is only one FileCheck invocation?

219 ↗

(On Diff #275695)

We might as well delete all the column: 1 fields, assuming that 0 is the default.

[vsk]

Are test{1,2,3,4,5,6} and main all necessary to exercise the changes in this patch? On the surface, it looks like there are two primary changes -- one that affects the case when deadargelim changes the function return type, and another that affects the case where deadargelim modifies a function that returns an array/struct. Can the test be pared down to just cover those two cases?

llvm/test/DebugInfo/X86/dbgloc-insert-extract-val-instrs.ll

4 ↗

(On Diff #275695)

It doesn't look like the -check-debugify output is important, so it shouldn't be necessary to run the pass.

Also, since the dbg.values are also not important, please run -debugify with -debugify-level=locations to omit those intrinsics.

7 ↗

(On Diff #275695)

Please restructure these checks so they have a clear correspondence to a test function. The typical way to write this is:

; CHECK-LABEL: some_test1
; CHECK: ...
define void @some_test1

; CHECK-LABEL: some_test2
; CHECK: ...
define void @some_test2

etc.

[djtodoro]

@aprantl @vsk Thanks a lot for the feedback!

llvm/test/DebugInfo/X86/dbgloc-insert-extract-val-instrs.ll

4 ↗

(On Diff #275695)

I see..thanks :)

5 ↗

(On Diff #275695)

Oh, sure..

7 ↗

(On Diff #275695)

I was a bit lazy, sure, thanks!

[djtodoro]

• Addressing comments
• Reduce the test

[djtodoro]

• [test] remove unused debugify metadata

[vsk]

Thanks, lgtm!

[djtodoro]

Thanks!

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

if (!CB.use_empty() || CB.isUsedByMetadata()) {
         // with all the uses, we will just rebuild it using extract/insertvalue
         // chaining and let instcombine clean that up.
         //
         // Start out building up our return value from undef
         Value *RetVal = UndefValue::get(RetTy);
         for (unsigned Ri = 0; Ri != RetCount; ++Ri)
           if (NewRetIdxs[Ri] != -1) {
             Value *V;
-            if (RetTypes.size() > 1)
+            if (RetTypes.size() > 1) {
               // We are still returning a struct, so extract the value from our
               // return value
               V = ExtractValueInst::Create(NewCB, NewRetIdxs[Ri], "newret",
                                            InsertPt);
-            else
+              cast<Instruction>(V)->setDebugLoc(InsertPt->getDebugLoc());
+            } else {
               // We are now returning a single element, so just insert that
               V = NewCB;
+            }
             // Insert the value at the old position
             RetVal = InsertValueInst::Create(RetVal, V, Ri, "oldret", InsertPt);
+            cast<Instruction>(RetVal)->setDebugLoc(InsertPt->getDebugLoc());
           }
         // Now, replace all uses of the old call instruction with the return
         // struct we built
         CB.replaceAllUsesWith(RetVal);
         NewCB->takeName(&CB);
       }
     }
 
for (BasicBlock &BB : *NF)
           // clean that up.
           Value *OldRet = RI->getOperand(0);
           // Start out building up our return value from undef
           RetVal = UndefValue::get(NRetTy);
           for (unsigned RetI = 0; RetI != RetCount; ++RetI)
             if (NewRetIdxs[RetI] != -1) {
               ExtractValueInst *EV =
                   ExtractValueInst::Create(OldRet, RetI, "oldret", RI);
+              EV->setDebugLoc(RI->getDebugLoc());
               if (RetTypes.size() > 1) {
                 // We're still returning a struct, so reinsert the value into
                 // our new return value at the new index
 
                 RetVal = InsertValueInst::Create(RetVal, EV, NewRetIdxs[RetI],
                                                  "newret", RI);
+                cast<Instruction>(RetVal)->setDebugLoc(RI->getDebugLoc());
               } else {
                 // We are now only returning a simple value, so just return the
                 // extracted value.
                 RetVal = EV;
               }
             }
         }
         // Replace the return instruction with one returning the new return

llvm/test/Transforms/DeadArgElim/multdeadretval.ll

 ; This test sees if return values (and arguments) are properly removed when they
 ; are unused. All unused values are typed i16, so we can easily check. We also
 ; run instcombine to fold insert/extractvalue chains and we run dce to clean up
 ; any remaining dead stuff.
 ; RUN: opt < %s -deadargelim -instcombine -dce -S | not grep i16
 
+; RUN: opt < %s -enable-debugify=synthetic -deadargelim -S 2>&1 \
+; RUN:     | FileCheck %s -check-prefix=DEBUG
+
+; Check that every instruction inserted by -deadargelim has a debug location.
+; DEBUG: CheckModuleDebugify: PASS
+
 define internal {i16, i32} @test(i16 %DEADARG) {
         %A = insertvalue {i16,i32} undef, i16 1, 0
         %B = insertvalue {i16,i32} %A, i32 1001, 1
         ret {i16,i32} %B
 }
 
 define internal {i32, i16} @test2() {
         %DEAD = call i16 @test4()