[SystemZ] Merge SystemZExpandPseudo pass into SystemZPostRewrite.

[jonpa]

Summary

SystemZExpandPseudo:s only job was to expand LOCRMux instructions into jump sequences. This needs to be done if expandLOCRPseudo() or expandSELRPseudo() fails to find a legal opcode (all registers "high" or "low"). This task has now been moved to SystemZPostRewrite while removing the SystemZExpandPseudo pass.

It is in fact preferred to expand these pseudos directly after register allocation in SystemZPostRewrite since the hinted register combinations are then not subject to later optimizations. A few more handled cases on SPEC 2006 can be seen with this: "Number of LOCRMux jump-sequences": z14: 5 -> 0, arch13: 5 -> 4.

I thought it was a bit simpler and cleaner to keep the expandLOCRPseudo() and expandSELRPseudo() in SystemZInstrInfo. Is it ok to use "public:" and "private:" like this, or should they be moved instead?

If the number of missed cases is a problem (4 on arch13), a next step to try would be the allowHintRecoloring() method as used in D58923, but perhaps that is not quite motivated given the near 0 number of cases left.

[uweigand]

I thought it was a bit simpler and cleaner to keep the expandLOCRPseudo() and expandSELRPseudo() in SystemZInstrInfo. Is it ok to use "public:" and "private:" like this, or should they be moved instead?

I'd prefer to move everything then.

lib/Target/SystemZ/SystemZPostRewrite.cpp

51

What does this change have to do with the rest of the patch?

[jonpa]

I moved the functions and renamed them trySelectLOCRMux() and trySelectSELRMux() to avoid confusion (they do not "expand" the LOCRMux). Is there a point in passing the opcodes as arguments to them?

This in turn made some other methods of SystemZInstrInfo required to be "public" in order to use them from SytemZPostRewrite. I made isHighReg() a static method of SystemZRegisterInfo instead, though.

Removed this from expandPostRAPseudo(), since it seemed a bit redundant:

case SystemZ::LOCRMux:
 case SystemZ::SELRMux:
   llvm_unreachable("Should have been handled in SystemZPostRewrite!");
   return true;

[jonpa]

Wait - is it safe to have SystemZRegisterInfo::isHighReg() be a static function? I remember that you said that static functions were bad in multi-threaded programs. Is the "Reg" argument safe if two threads call this method simultaneously?

lib/Target/SystemZ/SystemZPostRewrite.cpp

51

I figured that since expandLOCRMux() may create new basic blocks, we can't call setPreservesAll(), unless we would be willing to update any analysis passes as needed. I haven't looked into that in detail, but I imagine things like the Dominator Tree and LoopInfo should be recomputed / updated as needed in case expandLOCRMux() is called.

[uweigand]

I agree with making isHighReg a static function in SystemZRegisterInfo. I don't see why there would be any issue with a static member function, as long as it has no persistent state. The only issue with multi-threaded programs would be persistant state, e.g. a static member variable or a static variable within the member function. Arguments and (nonstatic) local variables are fine.

I'm not sure we need to expose emitGRX32Move though. In SystemZPostRewrite, can't we just use a COPY to copy values, like we already do elsewhere in this file?

Also, now that selection to existing ISA instruction and expansion to an if-then-else block happen at the same place, I think we can therefore simplify the logic a bit. For example, make expandLOCRMux into a helper routine that creates the if-then-else expansion, and rename trySelectLOCRMux and trySelectSELRMux to selectLOCRMux and selectSELRMux, and have then call the helper if they cannot select to an existing ISA routine directly. This will also avoid first replacing a SELRMux with a LOCRMux just to have the latter being deleted immediately afterwards. (As an additional potential simplication, if the if-then-else expansion helper were to directly support three-address moves, then some of logic in selectSELRMux could be simplified even further.)

lib/Target/SystemZ/SystemZPostRewrite.cpp

51

Ah, this makes sense. Thanks!

[jonpa]

The only issue with multi-threaded programs would be persistant state...

OK, thanks for the explanation.

I'm not sure we need to expose emitGRX32Move though. In SystemZPostRewrite, can't we just use a COPY to copy values, like we already do elsewhere in this file?

Ah, sorry - of course... I also changed the call TII->copyPhysReg() to build a COPY (which was NFC on SPEC 2006).

(As an additional potential simplication, if the if-then-else expansion helper were to directly support three-address moves, then some of logic in selectSELRMux could be simplified even further.)

Since the copying into dest in selectSELRMux() serves both the purpose of increasing the number of selr/selfhr instructions as well as to prepare for expandCondMove(), it seems like the best place to have it to me. I first thought that the commutation should be moved to only be done if needed before calling expandCondMove(). However, I then one case appeared on SPEC 2006 where the commutation had made two SELRs look the same and therefore the CFG optimizer could transform the function to just have one SELR. So I am thinking that it probably is slightly preferred to "canonicalize" the SELRs and increase the chances of them becoming identical. This means I am not sure how this function could actually be simplified...

[uweigand]

In D67432#1669132, @jonpa wrote:

Since the copying into dest in selectSELRMux() serves both the purpose of increasing the number of selr/selfhr instructions as well as to prepare for expandCondMove(), it seems like the best place to have it to me. I first thought that the commutation should be moved to only be done if needed before calling expandCondMove(). However, I then one case appeared on SPEC 2006 where the commutation had made two SELRs look the same and therefore the CFG optimizer could transform the function to just have one SELR. So I am thinking that it probably is slightly preferred to "canonicalize" the SELRs and increase the chances of them becoming identical. This means I am not sure how this function could actually be simplified...

Ah, I see. This makes sense. The patch looks good to me then we with a just a couple of minor cleanups.

lib/Target/SystemZ/SystemZPostRewrite.cpp

71

Comment should be updated to specify that otherwise, a branch sequence is created.

92

Likewise. Also, MixedOpcode is no longer used and should be removed.

151

I guess this now just be "return true" and we no longer need the Modified variable.

lib/Target/SystemZ/SystemZRegisterInfo.cpp

76 ↗

(On Diff #220063)

Maybe this small enough so it would sense to move the definition to the header file, so it can be marked inline again.

[jonpa]

Updated per review.

lib/Target/SystemZ/SystemZRegisterInfo.cpp

76 ↗

(On Diff #220063)

I put it next to the other inline:d query methods in SystemZRegisterInfo.h. This makes the total name more manageable as well for the callers, right?

[uweigand]

This LGTM now, thanks!

[jonpa]

r371959

lib/Target/SystemZ/CMakeLists.txt

 
 add_public_tablegen_target(SystemZCommonTableGen)
 
 add_llvm_target(SystemZCodeGen
   SystemZAsmPrinter.cpp
   SystemZCallingConv.cpp
   SystemZConstantPoolValue.cpp
   SystemZElimCompare.cpp
-  SystemZExpandPseudo.cpp
   SystemZFrameLowering.cpp
   SystemZHazardRecognizer.cpp
   SystemZISelDAGToDAG.cpp
   SystemZISelLowering.cpp
   SystemZInstrInfo.cpp
   SystemZLDCleanup.cpp
   SystemZLongBranch.cpp
   SystemZMachineFunctionInfo.cpp

lib/Target/SystemZ/SystemZ.h

 static inline bool isImmHF(uint64_t Val) {
   return (Val & ~0xffffffff00000000ULL) == 0;
 }
 } // end namespace SystemZ
 
 FunctionPass *createSystemZISelDag(SystemZTargetMachine &TM,
                                    CodeGenOpt::Level OptLevel);
 FunctionPass *createSystemZElimComparePass(SystemZTargetMachine &TM);
-FunctionPass *createSystemZExpandPseudoPass(SystemZTargetMachine &TM);
 FunctionPass *createSystemZShortenInstPass(SystemZTargetMachine &TM);
 FunctionPass *createSystemZLongBranchPass(SystemZTargetMachine &TM);
 FunctionPass *createSystemZLDCleanupPass(SystemZTargetMachine &TM);
 FunctionPass *createSystemZPostRewritePass(SystemZTargetMachine &TM);
 FunctionPass *createSystemZTDCPass();
 } // end namespace llvm
 
 #endif

lib/Target/SystemZ/SystemZExpandPseudo.cpp

-//==-- SystemZExpandPseudo.cpp - Expand pseudo instructions -------*- C++ -*-=//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file contains a pass that expands pseudo instructions into target
-// instructions to allow proper scheduling and other late optimizations.  This
-// pass should be run after register allocation but before the post-regalloc
-// scheduling pass.
-//
-//===----------------------------------------------------------------------===//
-
-#include "SystemZ.h"
-#include "SystemZInstrInfo.h"
-#include "SystemZSubtarget.h"
-#include "llvm/CodeGen/LivePhysRegs.h"
-#include "llvm/CodeGen/MachineFunctionPass.h"
-#include "llvm/CodeGen/MachineInstrBuilder.h"
-using namespace llvm;
-
-#define SYSTEMZ_EXPAND_PSEUDO_NAME "SystemZ pseudo instruction expansion pass"
-
-namespace llvm {
-  void initializeSystemZExpandPseudoPass(PassRegistry&);
-}
-
-namespace {
-class SystemZExpandPseudo : public MachineFunctionPass {
-public:
-  static char ID;
-  SystemZExpandPseudo() : MachineFunctionPass(ID) {
-    initializeSystemZExpandPseudoPass(*PassRegistry::getPassRegistry());
-  }
-
-  const SystemZInstrInfo *TII;
-
-  bool runOnMachineFunction(MachineFunction &Fn) override;
-
-  StringRef getPassName() const override { return SYSTEMZ_EXPAND_PSEUDO_NAME; }
-
-private:
-  bool expandMBB(MachineBasicBlock &MBB);
-  bool expandMI(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
-                MachineBasicBlock::iterator &NextMBBI);
-  bool expandLOCRMux(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
-                     MachineBasicBlock::iterator &NextMBBI);
-};
-char SystemZExpandPseudo::ID = 0;
-}
-
-INITIALIZE_PASS(SystemZExpandPseudo, "systemz-expand-pseudo",
-                SYSTEMZ_EXPAND_PSEUDO_NAME, false, false)
-
-/// Returns an instance of the pseudo instruction expansion pass.
-FunctionPass *llvm::createSystemZExpandPseudoPass(SystemZTargetMachine &TM) {
-  return new SystemZExpandPseudo();
-}
-
-// MI is a load-register-on-condition pseudo instruction that could not be
-// handled as a single hardware instruction.  Replace it by a branch sequence.
-bool SystemZExpandPseudo::expandLOCRMux(MachineBasicBlock &MBB,
-                                        MachineBasicBlock::iterator MBBI,
-                                        MachineBasicBlock::iterator &NextMBBI) {
-  MachineFunction &MF = *MBB.getParent();
-  const BasicBlock *BB = MBB.getBasicBlock();
-  MachineInstr &MI = *MBBI;
-  DebugLoc DL = MI.getDebugLoc();
-  Register DestReg = MI.getOperand(0).getReg();
-  Register SrcReg = MI.getOperand(2).getReg();
-  unsigned CCValid = MI.getOperand(3).getImm();
-  unsigned CCMask = MI.getOperand(4).getImm();
-
-  LivePhysRegs LiveRegs(TII->getRegisterInfo());
-  LiveRegs.addLiveOuts(MBB);
-  for (auto I = std::prev(MBB.end()); I != MBBI; --I)
-    LiveRegs.stepBackward(*I);
-
-  // Splice MBB at MI, moving the rest of the block into RestMBB.
-  MachineBasicBlock *RestMBB = MF.CreateMachineBasicBlock(BB);
-  MF.insert(std::next(MachineFunction::iterator(MBB)), RestMBB);
-  RestMBB->splice(RestMBB->begin(), &MBB, MI, MBB.end());
-  RestMBB->transferSuccessors(&MBB);
-  for (auto I = LiveRegs.begin(); I != LiveRegs.end(); ++I)
-    RestMBB->addLiveIn(*I);
-
-  // Create a new block MoveMBB to hold the move instruction.
-  MachineBasicBlock *MoveMBB = MF.CreateMachineBasicBlock(BB);
-  MF.insert(std::next(MachineFunction::iterator(MBB)), MoveMBB);
-  MoveMBB->addLiveIn(SrcReg);
-  for (auto I = LiveRegs.begin(); I != LiveRegs.end(); ++I)
-    MoveMBB->addLiveIn(*I);
-
-  // At the end of MBB, create a conditional branch to RestMBB if the
-  // condition is false, otherwise fall through to MoveMBB.
-  BuildMI(&MBB, DL, TII->get(SystemZ::BRC))
-    .addImm(CCValid).addImm(CCMask ^ CCValid).addMBB(RestMBB);
-  MBB.addSuccessor(RestMBB);
-  MBB.addSuccessor(MoveMBB);
-
-  // In MoveMBB, emit an instruction to move SrcReg into DestReg,
-  // then fall through to RestMBB.
-  TII->copyPhysReg(*MoveMBB, MoveMBB->end(), DL, DestReg, SrcReg,
-                   MI.getOperand(2).isKill());
-  MoveMBB->addSuccessor(RestMBB);
-
-  NextMBBI = MBB.end();
-  MI.eraseFromParent();
-  return true;
-}
-
-/// If MBBI references a pseudo instruction that should be expanded here,
-/// do the expansion and return true.  Otherwise return false.
-bool SystemZExpandPseudo::expandMI(MachineBasicBlock &MBB,
-                                   MachineBasicBlock::iterator MBBI,
-                                   MachineBasicBlock::iterator &NextMBBI) {
-  MachineInstr &MI = *MBBI;
-  switch (MI.getOpcode()) {
-  case SystemZ::LOCRMux:
-    return expandLOCRMux(MBB, MBBI, NextMBBI);
-  default:
-    break;
-  }
-  return false;
-}
-
-/// Iterate over the instructions in basic block MBB and expand any
-/// pseudo instructions.  Return true if anything was modified.
-bool SystemZExpandPseudo::expandMBB(MachineBasicBlock &MBB) {
-  bool Modified = false;
-
-  MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
-  while (MBBI != E) {
-    MachineBasicBlock::iterator NMBBI = std::next(MBBI);
-    Modified |= expandMI(MBB, MBBI, NMBBI);
-    MBBI = NMBBI;
-  }
-
-  return Modified;
-}
-
-bool SystemZExpandPseudo::runOnMachineFunction(MachineFunction &MF) {
-  TII = static_cast<const SystemZInstrInfo *>(MF.getSubtarget().getInstrInfo());
-
-  bool Modified = false;
-  for (auto &MBB : MF)
-    Modified |= expandMBB(MBB);
-  return Modified;
-}
-

lib/Target/SystemZ/SystemZInstrInfo.h

class SystemZInstrInfo : public SystemZGenInstrInfo {
   void expandRIPseudo(MachineInstr &MI, unsigned LowOpcode, unsigned HighOpcode,
                       bool ConvertHigh) const;
   void expandRIEPseudo(MachineInstr &MI, unsigned LowOpcode,
                        unsigned LowOpcodeK, unsigned HighOpcode) const;
   void expandRXYPseudo(MachineInstr &MI, unsigned LowOpcode,
                        unsigned HighOpcode) const;
   void expandLOCPseudo(MachineInstr &MI, unsigned LowOpcode,
                        unsigned HighOpcode) const;
+public:
   void expandLOCRPseudo(MachineInstr &MI, unsigned LowOpcode,
                         unsigned HighOpcode) const;
   void expandSELRPseudo(MachineInstr &MI, unsigned LowOpcode,
                         unsigned HighOpcode, unsigned MixedOpcode) const;
+private:
   void expandZExtPseudo(MachineInstr &MI, unsigned LowOpcode,
                         unsigned Size) const;
   void expandLoadStackGuard(MachineInstr *MI) const;
 
   MachineInstrBuilder
   emitGRX32Move(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                 const DebugLoc &DL, unsigned DestReg, unsigned SrcReg,
                 unsigned LowLowOpcode, unsigned Size, bool KillSrc,

lib/Target/SystemZ/SystemZInstrInfo.cpp

case SystemZ::LOCMux:
     expandLOCPseudo(MI, SystemZ::LOC, SystemZ::LOCFH);
     return true;
 
   case SystemZ::LOCHIMux:
     expandLOCPseudo(MI, SystemZ::LOCHI, SystemZ::LOCHHI);
     return true;
 
   case SystemZ::LOCRMux:
-    expandLOCRPseudo(MI, SystemZ::LOCR, SystemZ::LOCFHR);
-    return true;
-
   case SystemZ::SELRMux:
-    expandSELRPseudo(MI, SystemZ::SELR, SystemZ::SELFHR,
-                         SystemZ::LOCRMux);
+    llvm_unreachable("Should have been handled in SystemZPostRewrite!");
     return true;
 
   case SystemZ::STCMux:
     expandRXYPseudo(MI, SystemZ::STC, SystemZ::STCH);
     return true;
 
   case SystemZ::STHMux:
     expandRXYPseudo(MI, SystemZ::STH, SystemZ::STHH);

lib/Target/SystemZ/SystemZPostRewrite.cpp

public:
   }
 
   const SystemZInstrInfo *TII;
 
   bool runOnMachineFunction(MachineFunction &Fn) override;
 
   StringRef getPassName() const override { return SYSTEMZ_POSTREWRITE_NAME; }
 
-  void getAnalysisUsage(AnalysisUsage &AU) const override {
-    AU.setPreservesAll();
-    MachineFunctionPass::getAnalysisUsage(AU);
-  }
-
 private:
+  bool expandLOCRMux(MachineBasicBlock &MBB,
+                     MachineBasicBlock::iterator MBBI,
+                     MachineBasicBlock::iterator &NextMBBI);
   bool selectMI(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                 MachineBasicBlock::iterator &NextMBBI);
   bool selectMBB(MachineBasicBlock &MBB);
 };
 
 char SystemZPostRewrite::ID = 0;
 
 } // end anonymous namespace
 
 INITIALIZE_PASS(SystemZPostRewrite, "systemz-post-rewrite",
                 SYSTEMZ_POSTREWRITE_NAME, false, false)
 
 /// Returns an instance of the Post Rewrite pass.
 FunctionPass *llvm::createSystemZPostRewritePass(SystemZTargetMachine &TM) {
   return new SystemZPostRewrite();
 }
 
+// MI is a load-register-on-condition pseudo instruction that could not be
+// handled as a single hardware instruction.  Replace it by a branch sequence.
+bool SystemZPostRewrite::expandLOCRMux(MachineBasicBlock &MBB,
+                                       MachineBasicBlock::iterator MBBI,
+                                       MachineBasicBlock::iterator &NextMBBI) {
+  MachineFunction &MF = *MBB.getParent();
+  const BasicBlock *BB = MBB.getBasicBlock();
+  MachineInstr &MI = *MBBI;
+  DebugLoc DL = MI.getDebugLoc();
+  Register DestReg = MI.getOperand(0).getReg();
+  Register SrcReg = MI.getOperand(2).getReg();
+  unsigned CCValid = MI.getOperand(3).getImm();
+  unsigned CCMask = MI.getOperand(4).getImm();
+
+  LivePhysRegs LiveRegs(TII->getRegisterInfo());
+  LiveRegs.addLiveOuts(MBB);
+  for (auto I = std::prev(MBB.end()); I != MBBI; --I)
+    LiveRegs.stepBackward(*I);
+
+  // Splice MBB at MI, moving the rest of the block into RestMBB.
+  MachineBasicBlock *RestMBB = MF.CreateMachineBasicBlock(BB);
+  MF.insert(std::next(MachineFunction::iterator(MBB)), RestMBB);
+  RestMBB->splice(RestMBB->begin(), &MBB, MI, MBB.end());
+  RestMBB->transferSuccessors(&MBB);
+  for (auto I = LiveRegs.begin(); I != LiveRegs.end(); ++I)
+    RestMBB->addLiveIn(*I);
+
+  // Create a new block MoveMBB to hold the move instruction.
+  MachineBasicBlock *MoveMBB = MF.CreateMachineBasicBlock(BB);
+  MF.insert(std::next(MachineFunction::iterator(MBB)), MoveMBB);
+  MoveMBB->addLiveIn(SrcReg);
+  for (auto I = LiveRegs.begin(); I != LiveRegs.end(); ++I)
+    MoveMBB->addLiveIn(*I);
+
+  // At the end of MBB, create a conditional branch to RestMBB if the
+  // condition is false, otherwise fall through to MoveMBB.
+  BuildMI(&MBB, DL, TII->get(SystemZ::BRC))
+    .addImm(CCValid).addImm(CCMask ^ CCValid).addMBB(RestMBB);
+  MBB.addSuccessor(RestMBB);
+  MBB.addSuccessor(MoveMBB);
+
+  // In MoveMBB, emit an instruction to move SrcReg into DestReg,
+  // then fall through to RestMBB.
+  TII->copyPhysReg(*MoveMBB, MoveMBB->end(), DL, DestReg, SrcReg,
+                   MI.getOperand(2).isKill());
+  MoveMBB->addSuccessor(RestMBB);
+
+  NextMBBI = MBB.end();
+  MI.eraseFromParent();
+  return true;
+}
+
 /// If MBBI references a pseudo instruction that should be selected here,
 /// do it and return true.  Otherwise return false.
 bool SystemZPostRewrite::selectMI(MachineBasicBlock &MBB,
                                   MachineBasicBlock::iterator MBBI,
                                   MachineBasicBlock::iterator &NextMBBI) {
+  bool Modified = false;
   MachineInstr &MI = *MBBI;
   unsigned Opcode = MI.getOpcode();
 
   // Note: If this could be done during regalloc in foldMemoryOperandImpl()
   // while also updating the LiveIntervals, there would be no need for the
   // MemFoldPseudo to begin with.
   int TargetMemOpcode = SystemZ::getTargetMemOpcode(Opcode);
   if (TargetMemOpcode != -1) {
     MI.setDesc(TII->get(TargetMemOpcode));
     MI.tieOperands(0, 1);
     Register DstReg = MI.getOperand(0).getReg();
     MachineOperand &SrcMO = MI.getOperand(1);
     if (DstReg != SrcMO.getReg()) {
       BuildMI(MBB, &MI, MI.getDebugLoc(), TII->get(SystemZ::COPY), DstReg)
         .addReg(SrcMO.getReg());
       SrcMO.setReg(DstReg);
       MemFoldCopies++;
     }
     return true;
   }
 
-  return false;
+  switch (Opcode) {
+  case SystemZ::LOCRMux:
+    TII->expandLOCRPseudo(MI, SystemZ::LOCR, SystemZ::LOCFHR);
+    Modified = true;
+    break;
+  case SystemZ::SELRMux:
+    TII->expandSELRPseudo(MI, SystemZ::SELR, SystemZ::SELFHR,
+                          SystemZ::LOCRMux);
+    Modified = true;
+    break;
+  }
+  if (MI.getOpcode() == SystemZ::LOCRMux)
+    Modified |= expandLOCRMux(MBB, MBBI, NextMBBI);
+
+  return Modified;
 }
 
 /// Iterate over the instructions in basic block MBB and select any
 /// pseudo instructions.  Return true if anything was modified.
 bool SystemZPostRewrite::selectMBB(MachineBasicBlock &MBB) {
   bool Modified = false;
 
   MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();

lib/Target/SystemZ/SystemZTargetMachine.cpp

 }
 
 void SystemZPassConfig::addPreSched2() {
   // PostRewrite needs to be run at -O0 also (in which case addPostRewrite()
   // is not called).
   if (getOptLevel() == CodeGenOpt::None)
     addPass(createSystemZPostRewritePass(getSystemZTargetMachine()));
 
-  addPass(createSystemZExpandPseudoPass(getSystemZTargetMachine()));
-
   if (getOptLevel() != CodeGenOpt::None)
     addPass(&IfConverterID);
 }
 
 void SystemZPassConfig::addPreEmitPass() {
   // Do instruction shortening before compare elimination because some
   // vector instructions will be shortened into opcodes that compare
   // elimination recognizes.