[Debugify] Accumulate the number of variables in debugify metadata

[asi-sc]

Summary

When a module contains more than one function, we should update debugify metadata
by increasing the number of variables in the function rather than overwritting it.

[asi-sc]

Erase old debugify metadata before revisiting the module

[asi-sc]

A gentle ping

[xiangzhangllvm]

could you add a small test case here ?

[xiangzhangllvm]

llvm/lib/CodeGen/MachineDebugify.cpp

165

LGTM here

176

Here is runOnModule not runOnFunction, so I think here should only has 1 "llvm.mir.debugify".
I think the process should be "run 1 time debugfy for a module" --> optimizations--> "debugfy check" --> "clear the debugify info before next debugify cycle"
Sorry for the code is a little long time for me.

[asi-sc]

Add a test, address review comments

[asi-sc]

llvm/lib/CodeGen/MachineDebugify.cpp

176

Yeah, I did it a little wrong. So, the root cause is that we don't strip llvm.mir.debugify metadata. In this patch I suggest just checking that we do not forget to remove the old metadata when re-running the pass. And I'll create a fix to stripDebugifyMetadata to strip not only llvm.debugify but llvm.mir.debugify as well.

[xiangzhangllvm]

llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir

1

It is strange we check nothing, so how to judge this test is pass or not ?

[asi-sc]

llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir

1

We do check exit code. As you can see, we remove "xfail" in this patch. So, we had an assertion that is fixed by this patch:

asi@asi:~/dev/sources/llvm-project-mainstream/build-release$ bin/llvm-lit ../llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir  -a
-- Testing: 1 tests, 1 workers --
XFAIL: LLVM :: CodeGen/Generic/MIRDebugify/multifunction-module.mir (1 of 1)
Script:
--
: 'RUN: at line 1';   /home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc -run-pass=mir-debugify,mir-check-debugify /home/asi/dev/sources/llvm-project-mainstream/llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir
--
Exit Code: 134

Command Output (stderr):
--
+ : 'RUN: at line 1'
+ /home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc -run-pass=mir-debugify,mir-check-debugify /home/asi/dev/sources/llvm-project-mainstream/llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir
llc: /home/asi/dev/sources/llvm-project-mainstream/llvm/lib/CodeGen/MachineCheckDebugify.cpp:84: virtual bool {anonymous}::CheckDebugMachineModule::runOnModule(llvm::Module&): Assertion `Var <= NumVars && "Unexpected name for DILocalVariable"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc -run-pass=mir-debugify,mir-check-debugify /home/asi/dev/sources/llvm-project-mainstream/llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir
1.      Running pass 'Machine Check Debug Module' on module '/home/asi/dev/sources/llvm-project-mainstream/llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir'.
 #0 0x00005586602463c5 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #1 0x0000558660243ab4 SignalHandler(int) Signals.cpp:0:0
 #2 0x00007f74926dc520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #3 0x00007f7492730a7c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #4 0x00007f7492730a7c __pthread_kill_internal ./nptl/pthread_kill.c:78:10
 #5 0x00007f7492730a7c pthread_kill ./nptl/pthread_kill.c:89:10
 #6 0x00007f74926dc476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #7 0x00007f74926c27f3 abort ./stdlib/abort.c:81:7
 #8 0x00007f74926c271b _nl_load_domain ./intl/loadmsgcat.c:1177:9
 #9 0x00007f74926d3e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
#10 0x000055865f25babf (anonymous namespace)::CheckDebugMachineModule::runOnModule(llvm::Module&) MachineCheckDebugify.cpp:0:0
#11 0x000055865f7e4415 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc+0x5b83415)
#12 0x000055865d3de27f compileModule(char**, llvm::LLVMContext&) llc.cpp:0:0
#13 0x000055865d3df116 main (/home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc+0x377e116)
#14 0x00007f74926c3d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#15 0x00007f74926c3e40 call_init ./csu/../csu/libc-start.c:128:20
#16 0x00007f74926c3e40 __libc_start_main ./csu/../csu/libc-start.c:379:5
#17 0x000055865d3d1a25 _start (/home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc+0x3770a25)
/home/asi/dev/sources/llvm-project-mainstream/build-release/test/CodeGen/Generic/MIRDebugify/Output/multifunction-module.mir.script: line 1: 11064 Aborted                 (core dumped) /home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc -run-pass=mir-debugify,mir-check-debugify /home/asi/dev/sources/llvm-project-mainstream/llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir

I can precommit the test if you want to run it.
As for checks, I don't think we must once again check debugify functionality as we already have a few tests targeted it. It should be enough to verify that llc finishes correctly.

[xiangzhangllvm]

llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir

1

Does this is standard way to check test ? I didn't see this way before.
Not sure the assert is allowed/expected in auto testing tools.

[asi-sc]

llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir

1

Not sure the assert is allowed/expected in auto testing tools.

XFAIL can we used for this purpose. You might want to check test/CodeGen/AMDGPU/infinite-loop-evergreen.ll as an example:

asi@asi:~/dev/sources/llvm-project-mainstream/build-release$ bin/llvm-lit ../llvm/test/CodeGen/AMDGPU/infinite-loop-evergreen.ll -a
-- Testing: 1 tests, 1 workers --
XFAIL: LLVM :: CodeGen/AMDGPU/infinite-loop-evergreen.ll (1 of 1)
Script:
--
: 'RUN: at line 3';   /home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc -march=r600 -mcpu=cypress < /home/asi/dev/sources/llvm-project-mainstream/llvm/test/CodeGen/AMDGPU/infinite-loop-evergreen.ll | /home/asi/dev/sources/llvm-project-mainstream/build-release/bin/FileCheck /home/asi/dev/sources/llvm-project-mainstream/llvm/test/CodeGen/AMDGPU/infinite-loop-evergreen.ll
--
Exit Code: 2

Command Output (stderr):
--
error: no check strings found with prefix 'CHECK:'
error: Extra register needed to handle CFG
llc: /home/asi/dev/sources/llvm-project-mainstream/llvm/lib/Target/AMDGPU/R600MachineCFGStructurizer.cpp:1010: int {anonymous}::R600MachineCFGStructurizer::mergeLoop(llvm::MachineLoop*): Assertion `!ExitingMBBs.empty() && "Infinite Loop not supported"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc -march=r600 -mcpu=cypress
1.      Running pass 'Function Pass Manager' on module '<stdin>'.
2.      Running pass 'AMDGPU Control Flow Graph structurizer Pass' on function '@inf_loop_irreducible_cfg'
 #0 0x000055da349f63c5 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #1 0x000055da349f3ab4 SignalHandler(int) Signals.cpp:0:0
 #2 0x00007fb1543cc520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #3 0x00007fb154420a7c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #4 0x00007fb154420a7c __pthread_kill_internal ./nptl/pthread_kill.c:78:10
 #5 0x00007fb154420a7c pthread_kill ./nptl/pthread_kill.c:89:10
 #6 0x00007fb1543cc476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #7 0x00007fb1543b27f3 abort ./stdlib/abort.c:81:7
 #8 0x00007fb1543b271b _nl_load_domain ./intl/loadmsgcat.c:1177:9
 #9 0x00007fb1543c3e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
#10 0x000055da3222c3bf (anonymous namespace)::R600MachineCFGStructurizer::loopendPatternMatch() R600MachineCFGStructurizer.cpp:0:0
#11 0x000055da3222f524 (anonymous namespace)::R600MachineCFGStructurizer::runOnMachineFunction(llvm::MachineFunction&) R600MachineCFGStructurizer.cpp:0:0
#12 0x000055da33a39595 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (.part.0) MachineFunctionPass.cpp:0:0
#13 0x000055da33f938e3 llvm::FPPassManager::runOnFunction(llvm::Function&) (/home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc+0x5b828e3)
#14 0x000055da33f93b19 llvm::FPPassManager::runOnModule(llvm::Module&) (/home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc+0x5b82b19)
#15 0x000055da33f94415 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc+0x5b83415)
#16 0x000055da31b8e27f compileModule(char**, llvm::LLVMContext&) llc.cpp:0:0
#17 0x000055da31b8f116 main (/home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc+0x377e116)
#18 0x00007fb1543b3d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#19 0x00007fb1543b3e40 call_init ./csu/../csu/libc-start.c:128:20
#20 0x00007fb1543b3e40 __libc_start_main ./csu/../csu/libc-start.c:379:5
#21 0x000055da31b81a25 _start (/home/asi/dev/sources/llvm-project-mainstream/build-release/bin/llc+0x3770a25)

Does this is standard way to check test ? I didn't see this way before.

It's not as common as FileCheck, because usually we'd like to verify exact match with some pattern. However, sometimes it's OK to check just successful execution, e.g. CodeGen/AMDGPU/write-register-vgpr-into-sgpr.ll or CodeGen/X86/pr38038.ll. In general, I was able to grep ~1500 tests that check llc return code.

[xiangzhangllvm]

Yes, We use XFAIL to mark a test as an expected failure. An XFAIL test will be successful if its execution fails, and will be a failure if its execution succeeds.
But now we are not expected failure. it is strange to test a test case by check nothing (only expect it not go to assert). I am not much sure about it.
@dsanders any suggestions ?

[asi-sc]

In D136949#3945729, @xiangzhangllvm wrote:

Yes, We use XFAIL to mark a test as an expected failure. An XFAIL test will be successful if its execution fails, and will be a failure if its execution succeeds.
But now we are not expected failure. it is strange to test a test case by check nothing (only expect it not go to assert). I am not much sure about it.
@dsanders any suggestions ?

Let me auto-generate checks then? I don't think we really need them here, however, I'm not strongly against. If it's the only problem, it does not worth such a long discussion :)

[xiangzhangllvm]

Auto gen is unneccesary.
Try use CHECK-NOT: "assert message" ?

[asi-sc]

Rebased and added test checks to make forward progress on the review.

[xiangzhangllvm]

LGTM, thanks for your contribution！

[asi-sc]

I had to revert the change as I mistakenly forgot to strip the test from various x86 info such as regs, etc. So, almost all targets failed to run the test correctly. I'll update it soon.

[asi-sc]

Fix the test

llvm/lib/CodeGen/MachineDebugify.cpp

if (!NMD) {
     addDebugifyOperand(VarSet.size());
   } else {
     assert(NMD->getNumOperands() == 2 &&
            "llvm.mir.debugify should have exactly 2 operands!");
     auto setDebugifyOperand = [&](unsigned Idx, unsigned N) {
       NMD->setOperand(Idx, MDNode::get(Ctx, ValueAsMetadata::getConstant(
                                                 ConstantInt::get(Int32Ty, N))));
     };
+    auto getDebugifyOperand = [&](unsigned Idx) {
+      return mdconst::extract<ConstantInt>(NMD->getOperand(Idx)->getOperand(0))
+          ->getZExtValue();
+    };
     // Set number of lines.
     setDebugifyOperand(0, NextLine - 1);
     // Set number of variables.
-    setDebugifyOperand(1, VarSet.size());
+    auto OldNumVars = getDebugifyOperand(1);
+    setDebugifyOperand(1, OldNumVars + VarSet.size());
   }
 
   return true;
 }
 
 /// ModulePass for attaching synthetic debug info to everything, used with the
 /// legacy module pass manager.
 struct DebugifyMachineModule : public ModulePass {
   bool runOnModule(Module &M) override {
+    // We will insert new debugify metadata, so erasing the old one.
+    assert(!M.getNamedMetadata("llvm.mir.debugify") &&
+           "llvm.mir.debugify metadata already exists! Strip it first");
     MachineModuleInfo &MMI =
         getAnalysis<MachineModuleInfoWrapperPass>().getMMI();
     return applyDebugifyMetadata(
         M, M.functions(),
         "ModuleDebugify: ", [&](DIBuilder &DIB, Function &F) -> bool {
           return applyDebugifyMetadataToMachineFunction(MMI, DIB, F);
         });
   }

llvm/test/CodeGen/Generic/MIRDebugify/multifunction-module.mir

-# RUN: llc -run-pass=mir-debugify,mir-check-debugify %s
+# RUN: llc -run-pass=mir-debugify,mir-check-debugify -o - %s 2>&1 | FileCheck %s
 
-# XFAIL: *
+# CHECK: Machine IR debug info check: PASS
+# CHECK-NOT: Assertion `Var <= NumVars && "Unexpected name for DILocalVariable"'
 
 --- |
   define i32 @foo(i32 %a0, i32 %a1, i32 %a2, i32 %a3) {
     %t0 = add i32 %a0, %a1
     %t1 = add i32 %t0, %a2
     %t2 = add i32 %t1, %a3
     ret i32 %t2
   }
 
   define i32 @bar(i32 %a0, i32 %a1, i32 %a2, i32 %a3) {
     %t0 = add i32 %a0, %a1
     %t1 = add i32 %a2, %t0
     %t2 = add i32 %t1, %a3
     ret i32 %t2
   }
 
 ...
 ---
 name:            foo
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-hasWinCFI:       false
-callsEHReturn:   false
-callsUnwindInit: false
-hasEHCatchret:   false
-hasEHScopes:     false
-hasEHFunclets:   false
-failsVerification: false
-tracksDebugUserValues: false
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-  - { id: 3, class: _, preferred-register: '' }
-  - { id: 4, class: _, preferred-register: '' }
-  - { id: 5, class: _, preferred-register: '' }
-  - { id: 6, class: _, preferred-register: '' }
-liveins:
-  - { reg: '$edi', virtual-reg: '' }
-  - { reg: '$esi', virtual-reg: '' }
-  - { reg: '$edx', virtual-reg: '' }
-  - { reg: '$ecx', virtual-reg: '' }
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       0
-  offsetAdjustment: 0
-  maxAlignment:    1
-  adjustsStack:    false
-  hasCalls:        false
-  stackProtector:  ''
-  functionContext: ''
-  maxCallFrameSize: 4294967295
-  cvBytesOfCalleeSavedRegisters: 0
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-  hasTailCall:     false
-  localFrameSize:  0
-  savePoint:       ''
-  restorePoint:    ''
-fixedStack:      []
-stack:           []
-callSites:       []
-debugValueSubstitutions: []
-constants:       []
-machineFunctionInfo: {}
 body:             |
   bb.1 (%ir-block.0):
-    liveins: $ecx, $edi, $edx, $esi
-
-    %0:_(s32) = COPY $edi
-    %1:_(s32) = COPY $esi
-    %2:_(s32) = COPY $edx
-    %3:_(s32) = COPY $ecx
+    %0:_(s32) = IMPLICIT_DEF
+    %1:_(s32) = IMPLICIT_DEF
+    %2:_(s32) = IMPLICIT_DEF
+    %3:_(s32) = IMPLICIT_DEF
     %4:_(s32) = G_ADD %0, %1
     %5:_(s32) = G_ADD %4, %2
     %6:_(s32) = G_ADD %5, %3
-    $eax = COPY %6(s32)
-    RET 0, implicit $eax
 
 ...
 ---
 name:            bar
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-hasWinCFI:       false
-callsEHReturn:   false
-callsUnwindInit: false
-hasEHCatchret:   false
-hasEHScopes:     false
-hasEHFunclets:   false
-failsVerification: false
-tracksDebugUserValues: false
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-  - { id: 3, class: _, preferred-register: '' }
-  - { id: 4, class: _, preferred-register: '' }
-  - { id: 5, class: _, preferred-register: '' }
-  - { id: 6, class: _, preferred-register: '' }
-liveins:
-  - { reg: '$edi', virtual-reg: '' }
-  - { reg: '$esi', virtual-reg: '' }
-  - { reg: '$edx', virtual-reg: '' }
-  - { reg: '$ecx', virtual-reg: '' }
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       0
-  offsetAdjustment: 0
-  maxAlignment:    1
-  adjustsStack:    false
-  hasCalls:        false
-  stackProtector:  ''
-  functionContext: ''
-  maxCallFrameSize: 4294967295
-  cvBytesOfCalleeSavedRegisters: 0
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-  hasTailCall:     false
-  localFrameSize:  0
-  savePoint:       ''
-  restorePoint:    ''
-fixedStack:      []
-stack:           []
-callSites:       []
-debugValueSubstitutions: []
-constants:       []
-machineFunctionInfo: {}
 body:             |
   bb.1 (%ir-block.0):
-    liveins: $ecx, $edi, $edx, $esi
-
-    %0:_(s32) = COPY $edi
-    %1:_(s32) = COPY $esi
-    %2:_(s32) = COPY $edx
-    %3:_(s32) = COPY $ecx
+    %0:_(s32) = IMPLICIT_DEF
+    %1:_(s32) = IMPLICIT_DEF
+    %2:_(s32) = IMPLICIT_DEF
+    %3:_(s32) = IMPLICIT_DEF
     %4:_(s32) = G_ADD %0, %1
     %5:_(s32) = G_ADD %2, %4
     %6:_(s32) = G_ADD %5, %3
-    $eax = COPY %6(s32)
-    RET 0, implicit $eax
 
 ...