X86InstrInfo: Do not stop on flag users between flag producers

[MatzeB]

Summary

optimizeCompareInstr would stop searching if there are other EFLAG readers between the CmpInstr that we attempt to remove and the earlier instruction that we try to use instead. Example:

= SUB32rr %0, %1, implicit-def $eflags
...  we no longer stop when there are $eflag users here
CMP32rr %0, %1   ; will be removed
...

I don't see any reason why we cannot allow EFLAG readers between the two flag producers. Maybe it was an accident/artifact that wasn't obvious before the cleanups in D110857

[pengfei]

I have concern about the live interval of eflags. If we killed the eflags after the first use, we are free to insert or schedule instructions that implicit-def eflags. Will these be affected if we keep eflags alive?

[MatzeB]

In D110863#3036078, @pengfei wrote:

I have concern about the live interval of eflags. If we killed the eflags after the first use, we are free to insert or schedule instructions that implicit-def eflags. Will these be affected if we keep eflags alive?

When searching for candidates in the main search (the for loops in line 4298/4299) we never search further than the first instruction that modifies EFLAGS (see line 4351).

[MatzeB]

In D110863#3036078, @pengfei wrote:

I have concern about the live interval of eflags. If we killed the eflags after the first use, we are free to insert or schedule instructions that implicit-def eflags. Will these be affected if we keep eflags alive?

If you are worried about pre-existing kill-flags? We remove them in line 4497 when we actually perform the optimization.

[MatzeB]

ping

[pengfei]

LGTM, but I'd like others to sign off the patch.

[RKSimon]

LGTM - if you can it might be better to merge this into D110857?

[MatzeB]

LGTM - if you can it might be better to merge this into D110857?

I kept them separate so D110857 would be an NFC change. Anyway I'll merge them now.

[MatzeB]

Merged into D110857

llvm/lib/Target/X86/X86InstrInfo.cpp

for (MachineInstr &Inst : make_range(From, MBB->rend())) {
             Inst.registerDefIsDead(X86::EFLAGS, TRI)) {
           Movr0Inst = &Inst;
           continue;
         }
 
         // Cannot do anything for any other EFLAG changes.
         return false;
       }
-
-      if (Inst.readsRegister(X86::EFLAGS, TRI))
-        return false;
     }
 
     if (MI || Sub)
       break;
 
     // Reached begin of basic block. Continue in predecessor if there is
     // exactly one.
     if (MBB->pred_size() != 1)

llvm/test/CodeGen/X86/optimize-compare.mir

bb.0:
     %0:gr32 = COPY $esi
     %1:gr32 = COPY $edi
     %2:gr32 = SUB32rr %0, %1, implicit-def dead $eflags
     $eax = COPY %2
     ; CMP should be removed.
     CMP32rr %1, %0, implicit-def $eflags
     $bl = SETCCr 3, implicit $eflags
 ...
+---
+name: opt_redundant_flags_2
+body: |
+  bb.0:
+    ; CHECK-LABEL: name: opt_redundant_flags_2
+    ; CHECK: [[COPY:%[0-9]+]]:gr32 = COPY $esi
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:gr32 = COPY $edi
+    ; CHECK-NEXT: [[SUB32rr:%[0-9]+]]:gr32 = SUB32rr [[COPY]], [[COPY1]], implicit-def $eflags
+    ; CHECK-NEXT: $cl = SETCCr 2, implicit $eflags
+    ; CHECK-NEXT: $eax = COPY [[SUB32rr]]
+    ; CHECK-NEXT: $bl = SETCCr 2, implicit $eflags
+    %0:gr32 = COPY $esi
+    %1:gr32 = COPY $edi
+    %2:gr32 = SUB32rr %0, %1, implicit-def $eflags
+    ; an extra eflags reader shouldn't stop optimization.
+    $cl = SETCCr 2, implicit $eflags
+    $eax = COPY %2
+    CMP32rr %0, %1, implicit-def $eflags
+    $bl = SETCCr 2, implicit $eflags
+...