[X86] Remove EFLAGS from live-in lists of successor MBBs in X86FlagsCopyLowering

[jonpa]

Summary

This is an attempt to handle the problem discussed in https://reviews.llvm.org/D68267.

I haven't touched this is sometime. The thing I would worry about is whether *all* of the edges into the successor have been rewritten such that EFLAGS no longer needs to live-in. I don't recall whether the code ensures that is the case in a clear point, but if it does, that is the place to update the live-in list. (I suspect it does somewhere, and it is just finding it in order to do the update correctly... my guess is that it'll be at the end because we're operating edge-wise and may not have finished rewriting edges into the successor... but that's just a guess.)

As far as I can understand, X86FlagsCopyLoweringPass handles one COPY to X86::EFLAGS at a time by finding all its users. The users are found in the current MBB and all the successors (recursively) that has EFLAGS live-in. Each MBB is only visited once and the users are rewritten on the fly, except the jump instructions which are done after all else.

I don't see any part at all updating live-in lists, and I can't find any place in particular that checks that all predecessors of an MBB has been visited. Given that the jumps are done at the end, it would have to be after that, but there is nothing there...

I am not quite sure how this all works, but it seems that EFLAGS is copied into a virtual register (or several - saved in CondRegs) which the users are supposed to rely on, which would then mean that EFLAGS is in fact never live into any MBB with a user found here anymore after this pass?

I made this patch based on this idea which "seems to work", but I am not at all sure this is correct, so please take a look...

The other idea I had was to simply recompute the live-in lists from scratch after everything else, possibly as a new method of LivePhysRegs.

[jonpa]

ping!

[craig.topper]

llvm/lib/Target/X86/X86FlagsCopyLowering.cpp

549

I don't think we can do this here. Later we check for successor blocks with livein eflags and I believe we expect to be able to get back to the original MBB that started the worklist as second time if there is a loop. We don't put the initial block into the visited set until its queued as a successor. But we need the eflags livein in order to queue it.

554

Here the comments talk about visiting the block twice.

[jonpa]

Patch updated -- see inline comment for discussion.

llvm/lib/Target/X86/X86FlagsCopyLowering.cpp

549

Aha. My suggestion here did not update the live-in list for the original block, but it seems that this must then also be done at some point, which it is not...

It seems to work better then to do this when finding the successors, so that any successor with a live in EFLAGS gets it removed directly from the live-in list. This would then happen for MBB only inside a cycle when it is reached as a successor.

This seems good to me, while I still wonder if there would be any remaining cases of EFLAGS being live-in after this pass? If not, an alternative might be to just remove it from all live-in list at the end after all else.

[craig.topper]

LGTM

llvm/lib/Target/X86/X86FlagsCopyLowering.cpp

for (MachineInstr *CopyI : Copies) {
     // EFLAGS copy lives into successors.
     SmallVector<MachineBasicBlock *, 2> Blocks;
     SmallPtrSet<MachineBasicBlock *, 2> VisitedBlocks;
     Blocks.push_back(&MBB);
 
     do {
       MachineBasicBlock &UseMBB = *Blocks.pop_back_val();
 
       // Track when if/when we find a kill of the flags in this block.
       bool FlagsKilled = false;
 
       // In most cases, we walk from the beginning to the end of the block. But
       // when the block is the same block as the copy is from, we will visit it
       // twice. The first time we start from the copy and go to the end. The
       // second time we start from the beginning and go to the copy. This lets
       // us handle copies inside of cycles.
       // FIXME: This loop is *super* confusing. This is at least in part
       // a symptom of all of this routine needing to be refactored into
       // documentable components. Once done, there may be a better way to write
       // this loop.
       for (auto MII = (&UseMBB == &MBB && !VisitedBlocks.count(&UseMBB))
                           ? std::next(CopyI->getIterator())
do {
               SuccMBB->dump();
             });
             report_fatal_error(
                 "Cannot lower EFLAGS copy when original copy def "
                 "does not dominate all uses.");
           }
 
           Blocks.push_back(SuccMBB);
+
+          // After this, EFLAGS will be recreated before each use.
+          SuccMBB->removeLiveIn(X86::EFLAGS);
         }
     } while (!Blocks.empty());
 
     // Now rewrite the jumps that use the flags. These we handle specially
     // because if there are multiple jumps in a single basic block we'll have
     // to do surgery on the CFG.
     MachineBasicBlock *LastJmpMBB = nullptr;
     for (MachineInstr *JmpI : JmpIs) {

llvm/test/CodeGen/X86/copy-eflags-liveinlists.mir

+# RUN: llc -mtriple=i686-unknown-unknown -run-pass=x86-flags-copy-lowering \
+# RUN:  -print-after=x86-flags-copy-lowering %s -o - | FileCheck %s
+#
+# Check that $eflags is removed from live-in lists of successor blocks.
+#
+# CHECK-NOT: liveins: $eflags
+
+--- |
+  define void @fun(i16 %arg, i64 %arg1, i8 %arg2, i8* %arg3, i32 %arg4) { ret void}
+...
+---
+name:            fun
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr16 }
+  - { id: 1, class: gr16 }
+  - { id: 2, class: gr16 }
+  - { id: 3, class: gr32 }
+  - { id: 4, class: gr32 }
+  - { id: 5, class: gr8 }
+  - { id: 6, class: gr32 }
+  - { id: 7, class: gr32 }
+  - { id: 8, class: gr32 }
+  - { id: 9, class: gr32 }
+  - { id: 10, class: gr32 }
+  - { id: 11, class: gr32 }
+  - { id: 12, class: gr32 }
+  - { id: 13, class: gr8 }
+  - { id: 14, class: gr32 }
+  - { id: 15, class: gr32 }
+  - { id: 16, class: gr32_abcd }
+  - { id: 17, class: gr8 }
+  - { id: 18, class: gr8 }
+  - { id: 19, class: gr32 }
+  - { id: 20, class: gr32 }
+frameInfo:
+  maxAlignment:    4
+fixedStack:
+  - { id: 0, offset: 20, size: 4, alignment: 4, isImmutable: true }
+  - { id: 1, offset: 16, size: 4, alignment: 4, isImmutable: true }
+  - { id: 2, offset: 12, size: 1, alignment: 4, isImmutable: true }
+  - { id: 3, offset: 8, size: 4, alignment: 4, isImmutable: true }
+  - { id: 4, offset: 4, size: 4, alignment: 4, isImmutable: true }
+  - { id: 5, size: 2, alignment: 4, isImmutable: true }
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    %4:gr32 = MOV32rm %fixed-stack.3, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.3)
+    %3:gr32 = MOV32rm %fixed-stack.4, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.4)
+    %7:gr32 = MOV32rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.0)
+    %6:gr32 = MOV32rm %fixed-stack.1, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.1)
+    %5:gr8 = MOV8rm %fixed-stack.2, 1, $noreg, 0, $noreg :: (load 1 from %fixed-stack.2, align 4)
+    %9:gr32 = IMPLICIT_DEF
+    %11:gr32 = IMPLICIT_DEF
+  
+  bb.1:
+    successors: %bb.2, %bb.3
+  
+    CMP32rr %3, %9, implicit-def $eflags
+    %10:gr32 = SBB32rr %4, %11, implicit-def $eflags, implicit $eflags
+    %12:gr32 = COPY $eflags
+    %13:gr8 = SETCCr 12, implicit $eflags
+    %14:gr32 = MOVZX32rr8 killed %13
+    %15:gr32 = NEG32r %14, implicit-def dead $eflags
+    %16:gr32_abcd = MOV32r0 implicit-def dead $eflags
+    $eflags = COPY %12
+    %17:gr8 = COPY %16.sub_8bit
+    JCC_1 %bb.3, 12, implicit $eflags
+  
+  bb.2:
+    liveins: $eflags
+  
+  
+  bb.3:
+    successors: %bb.4, %bb.5
+    liveins: $eflags
+  
+    %18:gr8 = PHI %5, %bb.2, %17, %bb.1
+    MOV8mr %6, 1, $noreg, 0, $noreg, killed %18 :: (volatile store 1 into %ir.arg3)
+    JCC_1 %bb.5, 12, implicit $eflags
+  
+  bb.4:
+  
+  bb.5:
+    %19:gr32 = PHI %16, %bb.4, %15, %bb.3
+    $eax = COPY %7
+    CDQ implicit-def $eax, implicit-def $edx, implicit $eax
+    IDIV32r killed %19, implicit-def dead $eax, implicit-def $edx, implicit-def dead $eflags, implicit $eax, implicit $edx
+    JMP_1 %bb.1
+
+...