SystemZ: improved optimizeCompareZero()

[jonpa]

Summary

It was recently discovered that the ElimCompare pass can optimize:

%F2S<def> = LER %F0S
LTEBRCompare %F0S, %F0S, %CC<imp-def> LTEBRCompare %F0S, %F0S, %CC<imp-def>

but not:

LTEBRCompare %F0S, %F0S, %CC<imp-def> LTEBRCompare %F0S, %F0S, %CC<imp-def>
%F2S<def> = LER %F0S

My first thought was to swap the instructions as a setup for the algorithm, but then I realised I would still have
to do a search to do so, so I instead simply added the loop to do the forward search.

Have not yet checked if adjustCCMasksForInstr() can also be called here, but the calling fucntion is doing a
bottom-up traversal of MBB, so this might not work. For load-and-test, a top-down search here seems ok, I think.

Also fixed the comment at resultTests() which I thought was incomplete.

[uweigand]

The code change looks good to me, but please try to simplify the .mir test case following the suggestions outlined here: https://llvm.org/docs/MIRLangRef.html#simplifying-mir-files

[jonpa]

Test case reduced.

Also found out that the first check in resultTests() must not be used in a forward search, as in that case the reg is actually taking on a new value in between the compare and the CC-user. For this purpse, I added the 'BeforeCompare' parameter to the function.

I also wonder if the first check in resultTests() should include a check for a CC<def> operand?

[jonpa]

Opcode differences:

                  master                                             patched                                     
lgr            : 343498                                             342961                                                   -537
ltgr           : 4756                                               5293                                                     +537
jlh            : 48677                                              49178                                                    +501
cgijlh         : 18083                                              17600                                                    -483
ltr            : 218                                                460                                                      +242
lr             : 25730                                              25488                                                    -242
chi            : 15183                                              14995                                                    -188
cghi           : 4056                                               4019                                                      -37
je             : 94826                                              94848                                                     +22
...

[uweigand]

In D38076#877398, @jonpa wrote:

Test case reduced.

Also found out that the first check in resultTests() must not be used in a forward search, as in that case the reg is actually taking on a new value in between the compare and the CC-user. For this purpse, I added the 'BeforeCompare' parameter to the function.

Ah, right, this doesn't quite fit. The "resultTests()" routine currently is intended to check the following question: if MI were to be changed to an instruction that does the same as it does now, and also sets CC based on the result of the operation, would then that CC value also reflect the status of "Reg". This is true if either MI actually sets Reg, or else uses Reg as input to a value-preserving operation (like a copy or extend).

But for the new code, you actually want to check for something else; you only look whether MI uses Reg as input and sets CC accordingly. So your change is functionally correct, but I think it would be clearer if the resultTest() routine were split into two parts:

• add a new routine called something like preservesValueOf(MI, Reg) which returns true if MI is an instruction whose output equals the value in Reg. (This would be the switch part in resultTests.)
• change resultTests to do the first check for using Reg as output, and then simply call preservesValueOf(MI, Reg)

In the new code you add, you'd simply only call preservesValueOf.

I also wonder if the first check in resultTests() should include a check for a CC<def> operand?

No, the point is that even if MI currently does not define CC, if it can be modified to add a CC check that CC result would reflect the status (0, <0, >0) of the correct value. *Whether* or not MI already sets CC or can be modified to do so, is then checked at some later point (e.g. in convertToLoadAndTest).

[jonpa]

But for the new code, you actually want to check for something else; you only look whether MI uses Reg as input and sets CC accordingly. So your change is functionally correct, but I think it would be clearer if the resultTest() routine were split into two parts:

yes, that seems better - done.

I also wonder if the first check in resultTests() should include a check for a CC<def> operand?

No, the point is that even if MI currently does not define CC, if it can be modified to add a CC check that CC result would reflect the status (0, <0, >0) of the correct value. *Whether* or not MI already sets CC or can be modified to do so, is then checked at some later point (e.g. in convertToLoadAndTest).

Ah, right.

[uweigand]

LGTM, thanks!

[jonpa]

r313877

lib/Target/SystemZ/SystemZElimCompare.cpp

 // Return true if CC is live out of MBB.
 static bool isCCLiveOut(MachineBasicBlock &MBB) {
   for (auto SI = MBB.succ_begin(), SE = MBB.succ_end(); SI != SE; ++SI)
     if ((*SI)->isLiveIn(SystemZ::CC))
       return true;
   return false;
 }
 
-// Return true if any CC result of MI would reflect the value of Reg.
+// Return true if any CC result of MI would reflect the value of Reg, or if
+// it can be converted to do so.
 static bool resultTests(MachineInstr &MI, unsigned Reg) {
   if (MI.getNumOperands() > 0 && MI.getOperand(0).isReg() &&
       MI.getOperand(0).isDef() && MI.getOperand(0).getReg() == Reg)
     return true;
 
   switch (MI.getOpcode()) {
   case SystemZ::LR:
   case SystemZ::LGR:
if (resultTests(MI, SrcReg)) {
       if ((!CCRefs && convertToLoadAndTest(MI)) ||
           (!CCRefs.Def && adjustCCMasksForInstr(MI, Compare, CCUsers))) {
         EliminatedComparisons += 1;
         return true;
       }
     }
     SrcRefs |= getRegReferences(MI, SrcReg);
     if (SrcRefs.Def)
-      return false;
+      break;
     CCRefs |= getRegReferences(MI, SystemZ::CC);
     if (CCRefs.Use && CCRefs.Def)
+      break;
+  }
+
+  // Also do a forward search to handle cases where an instruction after the
+  // compare can be converted like
+  //
+  // LTEBRCompare %F0S, %F0S, %CC<imp-def> LTEBRCompare %F0S, %F0S, %CC<imp-def>
+  // %F2S<def> = LER %F0S
+  //
+  MBBI = Compare, MBBE = MBB.end();
+  while (++MBBI != MBBE) {
+    MachineInstr &MI = *MBBI;
+    if (resultTests(MI, SrcReg)) {
+      // Try to eliminate Compare by reusing a CC result from MI.
+      if (convertToLoadAndTest(MI)) {
+        EliminatedComparisons += 1;
+        return true;
+      }
+    }
+    if (getRegReferences(MI, SrcReg).Def)
+      return false;
+    if (getRegReferences(MI, SystemZ::CC))
       return false;
   }
+
   return false;
 }
 
 // Try to fuse comparison instruction Compare into a later branch.
 // Return true on success and if Compare is therefore redundant.
 bool SystemZElimCompare::fuseCompareOperations(
     MachineInstr &Compare, SmallVectorImpl<MachineInstr *> &CCUsers) {
   // See whether we have a single branch with which to fuse.

test/CodeGen/SystemZ/fp-cmp-07.mir

+# RUN: llc -mtriple=s390x-linux-gnu -mcpu=z10 -no-integrated-as -start-after=block-placement %s -o - | FileCheck %s
+# Test that load-and-test is used
+
+--- |
+  ; ModuleID = '/home/ijonpan/tmp/tc_ltebr.ll'
+  source_filename = "/home/ijonpan/llvm/llvm-dev-2/test/CodeGen/SystemZ/fp-cmp-04.ll"
+  target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64"
+  target triple = "s390x--linux-gnu"
+  
+  define float @f15(float %val, float %dummy, float* %dest) #0 {
+  entry:
+    call void asm sideeffect "blah $0", "{f2}"(float %val)
+    %cmp = fcmp olt float %val, 0.000000e+00
+    br i1 %cmp, label %exit, label %store
+  
+  store:                                            ; preds = %entry
+    store float %val, float* %dest
+    br label %exit
+  
+  exit:                                             ; preds = %store, %entry
+    ret float %val
+  }
+  
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**) #1
+  
+  attributes #0 = { "target-cpu"="z10" }
+  attributes #1 = { nounwind }
+
+...
+
+# CHECK: ltebr %f2, %f0
+
+---
+name:            f15
+alignment:       2
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:       
+liveins:         
+  - { reg: '%f0s', virtual-reg: '' }
+  - { reg: '%r2d', virtual-reg: '' }
+frameInfo:       
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      
+stack:           
+constants:       
+body:             |
+  bb.0.entry:
+    successors: %bb.1.store(0x80000000)
+    liveins: %f0s, %r2d
+  
+    LTEBRCompare %f0s, %f0s, implicit-def %cc
+    %f2s = LER %f0s
+    INLINEASM $"blah $0", 1, 9, %f2s
+    CondReturn 15, 4, implicit %f0s, implicit %cc
+  
+  bb.1.store:
+    liveins: %f0s, %r2d
+  
+    STE %f0s, killed %r2d, 0, _ :: (store 4 into %ir.dest)
+    Return implicit %f0s
+
+...