[SystemZ] Add a mapping from "select register" to "load on condition" (2-addr).

[jonpa]

Summary

The SELR(Mux) instructions can be converted to two-address form as LOCR(Mux) instructions whenever one of the sources are the same reg as dest. By adding this mapping in getTwoOperandOpcode(), we get:

• Two address hints in getRegAllocationHints() for select register instructions.
• No effect in foldMemoryOperandImpl() as the select register instructions do not have a memory mapped instructions. This is possibly a potential improvement to be utilized as is currently done for the K (3-address) instructions.
• no need for special handling in SystemZShortenInst.cpp - shortenSelect() removed.

It seems that there is yet another part of the "high-muxes patch" that never got committed separately, namely the moving of the handling of two-address hints to be done before the GRX32 hints, in getRegAllocationHints(), which this patch also does.

Instruction mix effects on SPEC 2006:

selgrlh        :                 1728                  231    -1497
locgre         :                 1923                 3385    +1462
selgre         :                 1511                  753     -758
locgrlh        :                 1159                 1861     +702
selgrl         :                 1175                  499     -676
locrl          :                 1354                 1996     +642
selgrnle       :                  641                   82     -559
locgrle        :                  356                  905     +549
locgrhe        :                  779                 1320     +541
selrl          :                 1081                  541     -540
locgrl         :                  722                 1059     +337
selrh          :                  536                  209     -327
ahi            :                31094                31403     +309
ahik           :                26654                26351     -303
locrle         :                  502                  749     +247
locre          :                  761                  977     +216
selgrhe        :                  216                    4     -212
selgrh         :                  489                  277     -212
selre          :                  333                  162     -171
...

Number of spill instructions go up just a little in total:

master <> patch
Spill|Reload   :               187260               187339      +79
Copies         :               414305               414303       -2

This seems to be small enough to be considered random. It seems that the number of spilled live ranges in total are very slightly less, while the number of instructions emitted for spill/restore are very slightly more... I could not find any reason to suspect anything in particular with the test cases I tried to examine, and it should hopefully not be bad to hint (softly) a two-address register.

738 files are different.

[uweigand]

What is the effect of this part?

It seems that there is yet another part of the "high-muxes patch" that never got committed separately, namely the moving of the handling of two-address hints to be done before the GRX32 hints, in getRegAllocationHints(), which this patch also does.

In any case, this should better be done as a separate commit.

no need for special handling in SystemZShortenInst.cpp - shortenSelect() removed.

Do those really never match again now? But I agree it would be nice to remove those if possible.

lib/Target/SystemZ/SystemZInstrFormats.td

3230 ↗

(On Diff #224590)

I'd prefer to move this into CondBinaryRRF itself, so that it matches CondBinaryRRFPseudo again.

3265 ↗

(On Diff #224590)

Likewise for CondBinaryRRFa.

[jonpa]

Patch updated (SystemZInstrFormats.td).

What is the effect of this part?

It seems that there is yet another part of the "high-muxes patch" that never got committed separately, namely the moving of the handling of two-address hints to be done before the GRX32 hints, in getRegAllocationHints(), which this patch also does.

In any case, this should better be done as a separate commit.

The effect of applying just the change in getRegAllocationHints() to move the 2-address hints above GRX32 hints is on SPEC 2006 / z15:

22 files are different.

master<> patch
ahik           :                26654                26355     -299
ahi            :                31094                31393     +299
locrhe         :                  659                  470     -189
locrl          :                 1354                 1468     +114
selrl          :                 1081                 1156      +75
l              :                73744                73751       +7
lgr            :               345992               345998       +6
lr             :                26243                26248       +5
lg             :               374181               374177       -4
stg            :               140079               140076       -3
stfh           :                 3288                 3291       +3
a              :                13806                13803       -3
ar             :                19536                19539       +3
iihf           :                 2271                 2274       +3
llihl          :                 2123                 2120       -3
risbhg         :                 3387                 3384       -3
cg             :                10168                10166       -2
cgrjhe         :                 4899                 4901       +2
jhe            :                10882                10880       -2

Spill|Reload   :               187260               187264       +4
Copies         :               414305               414313       +8

It seems that some two-address hints now are passed for instructions that use the same regs as a SELRMux, so there are more AHI:s and less AHIK:s. By chance - since we are not handling the SELRMux instructions here - we also get a few more SELR:s and a few less LOCR:s.

I could pre-commit this, or we could perhaps do benchmarking on the full patch (I see some mixed minor variations in my own runs).

no need for special handling in SystemZShortenInst.cpp - shortenSelect() removed.

Do those really never match again now? But I agree it would be nice to remove those if possible.

I checked it once on 2006 and it was NFC to remove them. shortenSelect() changes opcode and ties the operands if it can - using commuteInstruction() if helpful - and this is exactly what the default handling for a TwoOperandOpcode does as well.

[jonpa]

lib/Target/SystemZ/SystemZInstrFormats.td

3230 ↗

(On Diff #224590)

ok, makes sense.

[jonpa]

Patch rebased, no manual changes needed...

[uweigand]

LGTM, thanks!

[jonpa]

Thanks for review!

9fcebad

llvm/lib/Target/SystemZ/SystemZInstrFormats.td

class CondBinaryRRF<string mnemonic, bits<16> opcode, RegisterOperand cls1,
   : InstRRFc<opcode, (outs cls1:$R1),
              (ins cls1:$R1src, cls2:$R2, cond4:$valid, cond4:$M3),
              mnemonic#"$M3\t$R1, $R2",
              [(set cls1:$R1, (z_select_ccmask cls2:$R2, cls1:$R1src,
                                               cond4:$valid, cond4:$M3))]> {
   let Constraints = "$R1 = $R1src";
   let DisableEncoding = "$R1src";
   let CCMaskLast = 1;
+  let NumOpsKey = !subst("loc", "sel", mnemonic);
+  let NumOpsValue = "2";
 }
 
 // Like CondBinaryRRF, but used for the raw assembly form.  The condition-code
 // mask is the third operand rather than being part of the mnemonic.
 class AsmCondBinaryRRF<string mnemonic, bits<16> opcode, RegisterOperand cls1,
                        RegisterOperand cls2>
   : InstRRFc<opcode, (outs cls1:$R1),
              (ins cls1:$R1src, cls2:$R2, imm32zx4:$M3),

 class CondBinaryRRFa<string mnemonic, bits<16> opcode, RegisterOperand cls1,
                     RegisterOperand cls2, RegisterOperand cls3>
   : InstRRFa<opcode, (outs cls1:$R1),
              (ins cls3:$R3, cls2:$R2, cond4:$valid, cond4:$M4),
              mnemonic#"$M4\t$R1, $R2, $R3",
              [(set cls1:$R1, (z_select_ccmask cls2:$R2, cls3:$R3,
                                               cond4:$valid, cond4:$M4))]> {
   let CCMaskLast = 1;
+  let NumOpsKey = mnemonic;
+  let NumOpsValue = "3";
 }
 
 // Like CondBinaryRRFa, but used for the raw assembly form.  The condition-code
 // mask is the third operand rather than being part of the mnemonic.
 class AsmCondBinaryRRFa<string mnemonic, bits<16> opcode, RegisterOperand cls1,
                         RegisterOperand cls2, RegisterOperand cls3>
   : InstRRFa<opcode, (outs cls1:$R1), (ins cls3:$R3, cls2:$R2, imm32zx4:$M4),
              mnemonic#"\t$R1, $R2, $R3, $M4", []>;

 
 // Like TestBinarySIL, but expanded later.
 class TestBinarySILPseudo<SDPatternOperator operator, ImmOpWithPattern imm>
   : Pseudo<(outs), (ins bdaddr12only:$BD1, imm:$I2),
            [(set CC, (operator bdaddr12only:$BD1, imm:$I2))]>;
 
 // Like CondBinaryRRF, but expanded after RA depending on the choice of
 // register.
-class CondBinaryRRFPseudo<RegisterOperand cls1, RegisterOperand cls2>
+class CondBinaryRRFPseudo<string mnemonic, RegisterOperand cls1,
+                          RegisterOperand cls2>
   : Pseudo<(outs cls1:$R1),
            (ins cls1:$R1src, cls2:$R2, cond4:$valid, cond4:$M3),
            [(set cls1:$R1, (z_select_ccmask cls2:$R2, cls1:$R1src,
                                             cond4:$valid, cond4:$M3))]> {
   let Constraints = "$R1 = $R1src";
   let DisableEncoding = "$R1src";
   let CCMaskLast = 1;
+  let NumOpsKey = !subst("loc", "sel", mnemonic);
+  let NumOpsValue = "2";
 }
 
 // Like CondBinaryRRFa, but expanded after RA depending on the choice of
 // register.
-class CondBinaryRRFaPseudo<RegisterOperand cls1, RegisterOperand cls2,
-                           RegisterOperand cls3>
+class CondBinaryRRFaPseudo<string mnemonic, RegisterOperand cls1,
+                           RegisterOperand cls2, RegisterOperand cls3>
   : Pseudo<(outs cls1:$R1),
            (ins cls3:$R3, cls2:$R2, cond4:$valid, cond4:$M4),
            [(set cls1:$R1, (z_select_ccmask cls2:$R2, cls3:$R3,
                                             cond4:$valid, cond4:$M4))]> {
   let CCMaskLast = 1;
+  let NumOpsKey = mnemonic;
+  let NumOpsValue = "3";
 }
 
 // Like CondBinaryRIE, but expanded after RA depending on the choice of
 // register.
 class CondBinaryRIEPseudo<RegisterOperand cls, ImmOpWithPattern imm>
   : Pseudo<(outs cls:$R1),
            (ins cls:$R1src, imm:$I2, cond4:$valid, cond4:$M3),
            [(set cls:$R1, (z_select_ccmask imm:$I2, cls:$R1src,

llvm/lib/Target/SystemZ/SystemZInstrInfo.td

 // Conditional move instructions
 //===----------------------------------------------------------------------===//
 
 let Predicates = [FeatureMiscellaneousExtensions3], Uses = [CC] in {
   // Select.
   let isCommutable = 1 in {
     // Expands to SELR or SELFHR or a branch-and-move sequence,
     // depending on the choice of registers.
-    def  SELRMux : CondBinaryRRFaPseudo<GRX32, GRX32, GRX32>;
+    def  SELRMux : CondBinaryRRFaPseudo<"selrmux", GRX32, GRX32, GRX32>;
     defm SELFHR  : CondBinaryRRFaPair<"selfhr", 0xB9C0, GRH32, GRH32, GRH32>;
     defm SELR    : CondBinaryRRFaPair<"selr",   0xB9F0, GR32, GR32, GR32>;
     defm SELGR   : CondBinaryRRFaPair<"selgr",  0xB9E3, GR64, GR64, GR64>;
   }
 
   // Define AsmParser extended mnemonics for each general condition-code mask.
   foreach V = [ "E", "NE", "H", "NH", "L", "NL", "HE", "NHE", "LE", "NLE",
                 "Z", "NZ", "P", "NP", "M", "NM", "LH", "NLH", "O", "NO" ] in {
let Predicates = [FeatureLoadStoreOnCond2], Uses = [CC] in {
   defm LOCHI   : CondBinaryRIEPair<"lochi",  0xEC42, GR32, imm32sx16>;
   defm LOCGHI  : CondBinaryRIEPair<"locghi", 0xEC46, GR64, imm64sx16>;
 
   // Move register on condition.  Matched via DAG pattern and
   // created by early if-conversion.
   let isCommutable = 1 in {
     // Expands to LOCR or LOCFHR or a branch-and-move sequence,
     // depending on the choice of registers.
-    def LOCRMux : CondBinaryRRFPseudo<GRX32, GRX32>;
+    def LOCRMux : CondBinaryRRFPseudo<"locrmux", GRX32, GRX32>;
     defm LOCFHR : CondBinaryRRFPair<"locfhr", 0xB9E0, GRH32, GRH32>;
   }
 
   // Load on condition.  Matched via DAG pattern.
   // Expands to LOC or LOCFH, depending on the choice of register.
   def LOCMux : CondUnaryRSYPseudo<simple_load, GRX32, 4>;
   defm LOCFH : CondUnaryRSYPair<"locfh", 0xEBE0, simple_load, GRH32, 4>;
 

llvm/lib/Target/SystemZ/SystemZRegisterInfo.cpp

SystemZRegisterInfo::getRegAllocationHints(unsigned VirtReg,
                                            const LiveRegMatrix *Matrix) const {
   const MachineRegisterInfo *MRI = &MF.getRegInfo();
   const SystemZSubtarget &Subtarget = MF.getSubtarget<SystemZSubtarget>();
   const TargetRegisterInfo *TRI = Subtarget.getRegisterInfo();
 
   bool BaseImplRetVal = TargetRegisterInfo::getRegAllocationHints(
       VirtReg, Order, Hints, MF, VRM, Matrix);
 
+  if (VRM != nullptr) {
+    // Add any two address hints after any copy hints.
+    SmallSet<unsigned, 4> TwoAddrHints;
+    for (auto &Use : MRI->reg_nodbg_instructions(VirtReg))
+      if (SystemZ::getTwoOperandOpcode(Use.getOpcode()) != -1) {
+        const MachineOperand *VRRegMO = nullptr;
+        const MachineOperand *OtherMO = nullptr;
+        const MachineOperand *CommuMO = nullptr;
+        if (VirtReg == Use.getOperand(0).getReg()) {
+          VRRegMO = &Use.getOperand(0);
+          OtherMO = &Use.getOperand(1);
+          if (Use.isCommutable())
+            CommuMO = &Use.getOperand(2);
+        } else if (VirtReg == Use.getOperand(1).getReg()) {
+          VRRegMO = &Use.getOperand(1);
+          OtherMO = &Use.getOperand(0);
+        } else if (VirtReg == Use.getOperand(2).getReg() &&
+                   Use.isCommutable()) {
+          VRRegMO = &Use.getOperand(2);
+          OtherMO = &Use.getOperand(0);
+        } else
+          continue;
+
+        auto tryAddHint = [&](const MachineOperand *MO) -> void {
+          Register Reg = MO->getReg();
+          Register PhysReg =
+            Register::isPhysicalRegister(Reg) ? Reg : VRM->getPhys(Reg);
+          if (PhysReg) {
+            if (MO->getSubReg())
+              PhysReg = getSubReg(PhysReg, MO->getSubReg());
+            if (VRRegMO->getSubReg())
+              PhysReg = getMatchingSuperReg(PhysReg, VRRegMO->getSubReg(),
+                                            MRI->getRegClass(VirtReg));
+            if (!MRI->isReserved(PhysReg) && !is_contained(Hints, PhysReg))
+              TwoAddrHints.insert(PhysReg);
+          }
+        };
+        tryAddHint(OtherMO);
+        if (CommuMO)
+          tryAddHint(CommuMO);
+      }
+    for (MCPhysReg OrderReg : Order)
+      if (TwoAddrHints.count(OrderReg))
+        Hints.push_back(OrderReg);
+  }
+
   if (MRI->getRegClass(VirtReg) == &SystemZ::GRX32BitRegClass) {
     SmallVector<unsigned, 8> Worklist;
     SmallSet<unsigned, 4> DoneRegs;
     Worklist.push_back(VirtReg);
     while (Worklist.size()) {
       unsigned Reg = Worklist.pop_back_val();
       if (!DoneRegs.insert(Reg).second)
         continue;
while (Worklist.size()) {
               return false;
             }
           }
         } // end CHIMux / CFIMux
       }
     }
   }
 
-  if (VRM == nullptr)
-    return BaseImplRetVal;
-
-  // Add any two address hints after any copy hints.
-  SmallSet<unsigned, 4> TwoAddrHints;
-  for (auto &Use : MRI->reg_nodbg_instructions(VirtReg))
-    if (SystemZ::getTwoOperandOpcode(Use.getOpcode()) != -1) {
-      const MachineOperand *VRRegMO = nullptr;
-      const MachineOperand *OtherMO = nullptr;
-      const MachineOperand *CommuMO = nullptr;
-      if (VirtReg == Use.getOperand(0).getReg()) {
-        VRRegMO = &Use.getOperand(0);
-        OtherMO = &Use.getOperand(1);
-        if (Use.isCommutable())
-          CommuMO = &Use.getOperand(2);
-      } else if (VirtReg == Use.getOperand(1).getReg()) {
-        VRRegMO = &Use.getOperand(1);
-        OtherMO = &Use.getOperand(0);
-      } else if (VirtReg == Use.getOperand(2).getReg() && Use.isCommutable()) {
-        VRRegMO = &Use.getOperand(2);
-        OtherMO = &Use.getOperand(0);
-      } else
-        continue;
-
-      auto tryAddHint = [&](const MachineOperand *MO) -> void {
-        Register Reg = MO->getReg();
-        Register PhysReg =
-            Register::isPhysicalRegister(Reg) ? Reg : VRM->getPhys(Reg);
-        if (PhysReg) {
-          if (MO->getSubReg())
-            PhysReg = getSubReg(PhysReg, MO->getSubReg());
-          if (VRRegMO->getSubReg())
-            PhysReg = getMatchingSuperReg(PhysReg, VRRegMO->getSubReg(),
-                                          MRI->getRegClass(VirtReg));
-          if (!MRI->isReserved(PhysReg) && !is_contained(Hints, PhysReg))
-            TwoAddrHints.insert(PhysReg);
-        }
-      };
-      tryAddHint(OtherMO);
-      if (CommuMO)
-        tryAddHint(CommuMO);
-    }
-  for (MCPhysReg OrderReg : Order)
-    if (TwoAddrHints.count(OrderReg))
-      Hints.push_back(OrderReg);
-
   return BaseImplRetVal;
 }
 
 const MCPhysReg *
 SystemZRegisterInfo::getCalleeSavedRegs(const MachineFunction *MF) const {
   const SystemZSubtarget &Subtarget = MF->getSubtarget<SystemZSubtarget>();
   if (MF->getFunction().getCallingConv() == CallingConv::GHC)
     return CSR_SystemZ_NoRegs_SaveList;

llvm/lib/Target/SystemZ/SystemZShortenInst.cpp

 
 private:
   bool shortenIIF(MachineInstr &MI, unsigned LLIxL, unsigned LLIxH);
   bool shortenOn0(MachineInstr &MI, unsigned Opcode);
   bool shortenOn01(MachineInstr &MI, unsigned Opcode);
   bool shortenOn001(MachineInstr &MI, unsigned Opcode);
   bool shortenOn001AddCC(MachineInstr &MI, unsigned Opcode);
   bool shortenFPConv(MachineInstr &MI, unsigned Opcode);
-  bool shortenSelect(MachineInstr &MI, unsigned Opcode);
 
   const SystemZInstrInfo *TII;
   const TargetRegisterInfo *TRI;
   LivePhysRegs LiveRegs;
 };
 
 char SystemZShortenInst::ID = 0;
 } // end anonymous namespace
MachineInstrBuilder(*MI.getParent()->getParent(), &MI)
         .add(Mode)
         .add(Src)
         .add(Suppress);
     return true;
   }
   return false;
 }
 
-// MI is a three-operand select instruction.  If one of the sources match
-// the destination, convert to the equivalent load-on-condition.
-bool SystemZShortenInst::shortenSelect(MachineInstr &MI, unsigned Opcode) {
-  if (MI.getOperand(0).getReg() == MI.getOperand(1).getReg()) {
-    MI.setDesc(TII->get(Opcode));
-    MI.tieOperands(0, 1);
-    return true;
-  }
-  if (MI.getOperand(0).getReg() == MI.getOperand(2).getReg()) {
-    TII->commuteInstruction(MI, false, 1, 2);
-    MI.setDesc(TII->get(Opcode));
-    MI.tieOperands(0, 1);
-    return true;
-  }
-  return false;
-}
-
 // Process all instructions in MBB.  Return true if something changed.
 bool SystemZShortenInst::processBlock(MachineBasicBlock &MBB) {
   bool Changed = false;
 
   // Set up the set of live registers at the end of MBB (live out)
   LiveRegs.clear();
   LiveRegs.addLiveOuts(MBB);
 
   // Iterate backwards through the block looking for instructions to change.
   for (auto MBBI = MBB.rbegin(), MBBE = MBB.rend(); MBBI != MBBE; ++MBBI) {
     MachineInstr &MI = *MBBI;
     switch (MI.getOpcode()) {
     case SystemZ::IILF:
       Changed |= shortenIIF(MI, SystemZ::LLILL, SystemZ::LLILH);
       break;
 
     case SystemZ::IIHF:
       Changed |= shortenIIF(MI, SystemZ::LLIHL, SystemZ::LLIHH);
       break;
 
-    case SystemZ::SELR:
-      Changed |= shortenSelect(MI, SystemZ::LOCR);
-      break;
-
-    case SystemZ::SELFHR:
-      Changed |= shortenSelect(MI, SystemZ::LOCFHR);
-      break;
-
-    case SystemZ::SELGR:
-      Changed |= shortenSelect(MI, SystemZ::LOCGR);
-      break;
-
     case SystemZ::WFADB:
       Changed |= shortenOn001AddCC(MI, SystemZ::ADBR);
       break;
 
     case SystemZ::WFASB:
       Changed |= shortenOn001AddCC(MI, SystemZ::AEBR);
       break;
 

llvm/test/CodeGen/SystemZ/cond-move-08.mir

 # RUN: llc -mtriple=s390x-linux-gnu -mcpu=z15 -start-before=greedy %s -o - \
 # RUN:   | FileCheck %s
 #
 # Test that regalloc manages (via regalloc hints) to avoid a LOCRMux jump
-# sequence expansion, and a SELR instuction is emitted.
+# sequence expansion, and a SELR/LOCR instuction is emitted.
 
 --- |
   ; ModuleID = 'tc.ll'
   source_filename = "tc.ll"
   target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64"
 
   @globvar = external global i32
 
--- |
   ; Function Attrs: nounwind
   declare void @llvm.stackprotector(i8*, i8**) #1
 
   attributes #0 = { "target-cpu"="z15" }
   attributes #1 = { nounwind }
 
 ...
 
-# CHECK: selr
+# CHECK: locr
 # CHECK-NOT: risblg
 
 ---
 name:            fun1
 alignment:       16
 tracksRegLiveness: true
 registers:
   - { id: 0, class: grx32bit }

llvm/test/CodeGen/SystemZ/cond-move-regalloc-hints-02.mir

+# RUN: llc -mtriple=s390x-linux-gnu -mcpu=z15 -start-before=greedy %s -o - \
+# RUN:   | FileCheck %s
+#
+# Test that two-address reg alloc hints are given so that a SELR becomes LOCR.
+
+
+--- |
+  define i32 @fun(i32 %arg, i32 %arg1, i32 %arg2, i32* %arg3) { ret i32 0 }
+  declare void @foo(i32)
+...
+# CHECK-LABEL: fun
+# CHECK: locr
+
+---
+name:            fun
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr32bit }
+  - { id: 1, class: gr32bit }
+  - { id: 2, class: gr32bit }
+  - { id: 3, class: gr32bit }
+  - { id: 4, class: gr64bit }
+  - { id: 5, class: grx32bit }
+  - { id: 6, class: grx32bit }
+  - { id: 7, class: addr64bit }
+  - { id: 8, class: grx32bit }
+  - { id: 9, class: grx32bit }
+  - { id: 10, class: gr64bit }
+  - { id: 11, class: gr32bit }
+frameInfo:
+  maxAlignment:    1
+  hasCalls:        true
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    %5:grx32bit = LHIMux 88
+    %8:grx32bit = LHIMux 77
+    %9:grx32bit = LHIMux 66
+  
+  bb.1:
+    %6:grx32bit = LLCMux undef %7:addr64bit, 0, $noreg :: (load 1 from `i8* undef`)
+    CHIMux %6, 1, implicit-def $cc
+    %11:gr32bit = SELRMux %8, %9:grx32bit, 14, 6, implicit killed $cc
+    CHIMux %6, 2, implicit-def $cc
+    %0:gr32bit = SELRMux %11, %5, 14, 8, implicit killed $cc
+    ADJCALLSTACKDOWN 0, 0
+    %10:gr64bit = LGFR %0
+    $r2d = COPY %10
+    CallBRASL @foo, killed $r2d, csr_systemz, implicit-def dead $r14d, implicit-def dead $cc, implicit $fpc
+    ADJCALLSTACKUP 0, 0
+    J %bb.1
+
+...