[PATCH] D68267: [MBB LiveIn lists, MachineVerifier, SystemZ] New method isLiveOut() and mverifier improvement.

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added a comment.

In D68267#1732715 <https://reviews.llvm.org/D68267#1732715>, @RKSimon wrote:

> @jonpa This is still causing problems with EXPENSIVE_CHECKS builds please can you take a look? http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/220/


It seems that the strengthened MachineVerifier checks have discovered two tests which in fact end up with a corrupt MIR (live-in lists) after optimizations. For the first one I found a simple solution I think, but the second one isn't quite that simple, perhaps:

- CodeGen/X86/implicit-null-checks.mir

It seems that "Implicit null checks" adds $eflags as live in to bb.1.not_null, which is wrong since the preceding definition of it is dead (by the ADD32rr):

  # *** IR Dump Before Implicit null checks ***:
  # Machine code for function inc_store_with_dep: IsSSA, NoPHIs, TracksLiveness, NoVRegs
  Function Live Ins: $rdi, $rsi
  
  bb.0.entry:
    successors: %bb.2(0x40000000), %bb.1(0x40000000); %bb.2(50.00%), %bb.1(50.00%)
    liveins: $rdi, $rsi
    TEST64rr $rdi, $rdi, implicit-def $eflags
    JCC_1 %bb.2, 4, implicit killed $eflags
  
  bb.1.not_null:
  ; predecessors: %bb.0
    liveins: $rdi, $rsi
    $esi = ADD32rr killed $esi(tied-def 0), killed $esi, implicit-def dead $eflags
    MOV32mr killed $rdi, 1, $noreg, 16, $noreg, killed $esi
    RETQ
  
  bb.2.is_null:
  ; predecessors: %bb.0
  
  # After Implicit null checks
  # Machine code for function inc_store_with_dep: IsSSA, NoPHIs, TracksLiveness, NoVRegs
  Function Live Ins: $rdi, $rsi
  
  bb.0.entry:
    successors: %bb.2(0x40000000), %bb.1(0x40000000); %bb.2(50.00%), %bb.1(50.00%)
    liveins: $rdi, $rsi
    $esi = ADD32rr killed $esi(tied-def 0), killed $esi, implicit-def dead $eflags
    $noreg = FAULTING_OP 3, %bb.2, 1664, $rdi, 1, $noreg, 16, $noreg, $esi
    JMP_1 %bb.1
  
  bb.1.not_null:
  ; predecessors: %bb.0
    liveins: $rdi, $rsi, $esi, $eflags
    RETQ
  
  bb.2.is_null:
  ; predecessors: %bb.0
  
    RETQ
  
  # End machine code for function inc_store_with_dep.
  
  *** Bad machine code: Live in register not found to be live out from predecessor. ***
  - function:    inc_store_with_dep
  - basic block: %bb.1 not_null (0x6471fe8)
  EFLAGS not found to be live out from %bb.0
  LLVM ERROR: Found 1 machine code errors.

My idea to fix this is to not add a dead definition from DepMI as live-in to NC.getNotNullSucc():

  diff --git a/llvm/lib/CodeGen/ImplicitNullChecks.cpp b/llvm/lib/CodeGen/ImplicitNullChecks.cpp
  index b7dcaec..85e49e0 100644
  --- a/llvm/lib/CodeGen/ImplicitNullChecks.cpp
  +++ b/llvm/lib/CodeGen/ImplicitNullChecks.cpp
  @@ -697,7 +697,7 @@ void ImplicitNullChecks::rewriteNullChecks(
   
       if (auto *DepMI = NC.getOnlyDependency()) {
         for (auto &MO : DepMI->operands()) {
  -        if (!MO.isReg() || !MO.getReg() || !MO.isDef())
  +        if (!MO.isReg() || !MO.getReg() || !MO.isDef() || MO.isDead())
             continue;
           if (!NC.getNotNullSucc()->isLiveIn(MO.getReg()))
             NC.getNotNullSucc()->addLiveIn(MO.getReg());



- CodeGen/X86/copy-eflags.ll

"X86 EFLAGS copy lowering" transforms the def-use lists of $eflags (to local def-uses) without updating the livein lists of successor blocks:

  bb.1.bb1:
    ...
    $eflags = COPY %16:gr32
    JCC_1 %bb.3, 12, implicit $eflags
  
  bb.2.bb1:
  ; predecessors: %bb.1
    successors: %bb.3(0x80000000); %bb.3(100.00%)
    liveins: $eflags
  
  bb.3.bb1:
  ; predecessors: %bb.1, %bb.2
    successors: %bb.4(0x40000000), %bb.5(0x40000000); %bb.4(50.00%), %bb.5(50.00%)
    liveins: $eflags
    %2:gr8 = PHI %8:gr8, %bb.2, %0:gr8, %bb.1
    MOV8mr %9:gr32, 1, $noreg, 0, $noreg, %2:gr8 :: (volatile store 1 into %ir.ptr1)
    %20:gr32 = MOV32rm %10:gr32, 1, $noreg, 0, $noreg :: (volatile load 4 from %ir.ptr2)
    JCC_1 %bb.5, 12, implicit $eflags
  
  =>
  
  After X86 EFLAGS copy lowering:
  
  bb.1.bb1:
    TEST8rr %23:gr8, %23:gr8, implicit-def $eflags
    JCC_1 %bb.3, 5, implicit killed $eflags
  
  bb.2.bb1:
  ; predecessors: %bb.1
    successors: %bb.3(0x80000000); %bb.3(100.00%)
    liveins: $eflags
  
  bb.3.bb1:
  ; predecessors: %bb.1, %bb.2
    successors: %bb.4(0x40000000), %bb.5(0x40000000); %bb.4(50.00%), %bb.5(50.00%)
    liveins: $eflags
    %2:gr8 = PHI %8:gr8, %bb.2, %0:gr8, %bb.1
    MOV8mr %9:gr32, 1, $noreg, 0, $noreg, %2:gr8 :: (volatile store 1 into %ir.ptr1)
    %20:gr32 = MOV32rm %10:gr32, 1, $noreg, 0, $noreg :: (volatile load 4 from %ir.ptr2)
    TEST8rr %23:gr8, %23:gr8, implicit-def $eflags
    JCC_1 %bb.5, 5, implicit killed $eflags

It seems that since X86FlagsCopyLoweringPass::rewriteCondJmp() does JmpI.findRegisterUseOperand(X86::EFLAGS)->setIsKill(true), it should be safe to assume that EFLAGS is not live out. In that case it should also follow that any successor block will not use a live-in value of EFLAGS. So any successor block should have EFLAGS removed of the live-in list, right?

I am not sure what is the right way to clear EFLAGS of the successor blocks live-in lists. Should it be done in rewriteCondJmp() only or are there other places also that this should be done?


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

https://reviews.llvm.org/D68267