Pass LiveIntervals to foldMemoryOperandImpl(), to enable checking of liveness of preg (flag reg).

[jonpa]

Summary

SystemZ (and probably other targets as well) can fold a memory operand by changing the opcode into a new instruction that as a side-effect also clobbers the CC-reg.

In order to do this, liveness of that reg must first be checked.

When LIS is passed, getRegUnit() can be called on it and the right LiveRange is computed on demand.

(originated on llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2016-April/098301.html)

[jonpa]

Also re-enable test case for the LA -> AGSI optimization that has been temporarily disabled until this is resolved.

[MatzeB]

Looks good in principle, but I am pretty sure you did not even compile this. (Invalid call as below, no target except SystemZ adapted).

lib/CodeGen/InlineSpiller.cpp

764–765

huh, does this compile with 3 arguments to the first foldMemoryOperand() call?

lib/Target/SystemZ/SystemZInstrInfo.cpp

864–865

You should add an assert here that SystemZ::CC does indeed only have a single register unit.

[MatzeB]

lib/Target/SystemZ/SystemZInstrInfo.cpp

866

This needs a .getRegSlot() added.

[jonpa]

I did compile it - but only for SystemZ so far. I will fix other targets as well - but let me first await your answer on the overloaded method.. Should that be extended also, you think (see comment)?

lib/CodeGen/InlineSpiller.cpp

764–765

Yes - thats a different (overloaded) method. Perhaps this should also be extended with LIS in the same way, for sake of conformity?

[MatzeB]

In D19861#421297, @jonpa wrote:

I did compile it - but only for SystemZ so far. I will fix other targets as well - but let me first await your answer on the overloaded method.. Should that be extended also, you think (see comment)?

Yes please go ahead.

lib/CodeGen/InlineSpiller.cpp

764–765

Oh that is confusing. We should rename one of them instead of overloading, but that is for a different patch. Yep it sounds sensible to add the extra parameter to both.

[jonpa]

Other targets methods modified as well.

I realized that LIS may not actually be available (e.g. when called from X86::optimizeLoadInstr()), so I changed the LIS argument into a pointer type with default value of nullptr.

Not sure if there is a better way to assert for single reg unit in SystemZInstrInfo::foldMemoryOperandImpl()?

Not sure if there is / should be a simpler way of checking liveness of a register at the point of an instruction. For example LIS->isRegisterLiveAtInstruction(SystemZ::CC, MI) and LIS->createDeadDefAt(System::CC, MI)?

[MatzeB]

In D19861#424384, @jonpa wrote:

Other targets methods modified as well.

I realized that LIS may not actually be available (e.g. when called from X86::optimizeLoadInstr()), so I changed the LIS argument into a pointer type with default value of nullptr.

LGTM

Not sure if there is a better way to assert for single reg unit in SystemZInstrInfo::foldMemoryOperandImpl()?

I don't know of any better either.

Not sure if there is / should be a simpler way of checking liveness of a register at the point of an instruction. For example LIS->isRegisterLiveAtInstruction(SystemZ::CC, MI) and LIS->createDeadDefAt(System::CC, MI)?

isRegisterLiveAtInstruction() would be too simplistic. Look at the existing LiveRange::Query() which gathers all liveness related information you may need about an instruction. We could add a convenience function around that (in another patch) though I I personally didn't miss it much and often needed the SlotIndex, LiveInterval anyway so the convenience method wouldn't have helped.

[jonpa]

Thanks for review, Matthias.

Commited as r269026.

include/llvm/Target/TargetInstrInfo.h

 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/CodeGen/MachineCombinerPattern.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/Support/BranchProbability.h"
 #include "llvm/Target/TargetRegisterInfo.h"
+#include "llvm/CodeGen/LiveIntervalAnalysis.h"
 
 namespace llvm {
 
 class InstrItineraryData;
 class LiveVariables;
 class MCAsmInfo;
 class MachineMemOperand;
 class MachineRegisterInfo;
public:
 
   /// Attempt to fold a load or store of the specified stack
   /// slot into the specified machine instruction for the specified operand(s).
   /// If this is possible, a new instruction is returned with the specified
   /// operand folded, otherwise NULL is returned.
   /// The new instruction is inserted before MI, and the client is responsible
   /// for removing the old instruction.
   MachineInstr *foldMemoryOperand(MachineBasicBlock::iterator MI,
-                                  ArrayRef<unsigned> Ops, int FrameIndex) const;
+                                  ArrayRef<unsigned> Ops, int FrameIndex,
+                                  LiveIntervals &LIS) const;
 
   /// Same as the previous version except it allows folding of any load and
   /// store from / to any address, not just from a specific stack slot.
   MachineInstr *foldMemoryOperand(MachineBasicBlock::iterator MI,
                                   ArrayRef<unsigned> Ops,
                                   MachineInstr *LoadMI) const;
 
   /// Return true when there is potentially a faster code sequence

 protected:
   /// Target-dependent implementation for foldMemoryOperand.
   /// Target-independent code in foldMemoryOperand will
   /// take care of adding a MachineMemOperand to the newly created instruction.
   /// The instruction and any auxiliary instructions necessary will be inserted
   /// at InsertPt.
   virtual MachineInstr *foldMemoryOperandImpl(
       MachineFunction &MF, MachineInstr *MI, ArrayRef<unsigned> Ops,
-      MachineBasicBlock::iterator InsertPt, int FrameIndex) const {
+      MachineBasicBlock::iterator InsertPt, int FrameIndex,
+      LiveIntervals &LIS) const {
     return nullptr;
   }
 
   /// Target-dependent implementation for foldMemoryOperand.
   /// Target-independent code in foldMemoryOperand will
   /// take care of adding a MachineMemOperand to the newly created instruction.
   /// The instruction and any auxiliary instructions necessary will be inserted
   /// at InsertPt.

lib/CodeGen/InlineSpiller.cpp

for (const auto &OpPair : Ops) {
     // Tied use operands should not be passed to foldMemoryOperand.
     if (!MI->isRegTiedToDefOperand(Idx))
       FoldOps.push_back(Idx);
   }
 
   MachineInstrSpan MIS(MI);
 
   MachineInstr *FoldMI =
                 LoadMI ? TII.foldMemoryOperand(MI, FoldOps, LoadMI)
-                       : TII.foldMemoryOperand(MI, FoldOps, StackSlot);
+                       : TII.foldMemoryOperand(MI, FoldOps, StackSlot, LIS);
   if (!FoldMI)
     return false;
 
   // Remove LIS for any dead defs in the original MI not in FoldMI.
   for (MIBundleOperands MO(*MI); MO.isValid(); ++MO) {
     if (!MO->isReg())
       continue;
     unsigned Reg = MO->getReg();

lib/CodeGen/TargetInstrInfo.cpp

 /// foldMemoryOperand - Attempt to fold a load or store of the specified stack
 /// slot into the specified machine instruction for the specified operand(s).
 /// If this is possible, a new instruction is returned with the specified
 /// operand folded, otherwise NULL is returned. The client is responsible for
 /// removing the old instruction and adding the new one in the instruction
 /// stream.
 MachineInstr *TargetInstrInfo::foldMemoryOperand(MachineBasicBlock::iterator MI,
                                                  ArrayRef<unsigned> Ops,
-                                                 int FI) const {
+                                                 int FI,
+                                                 LiveIntervals &LIS) const {
   unsigned Flags = 0;
   for (unsigned i = 0, e = Ops.size(); i != e; ++i)
     if (MI->getOperand(Ops[i]).isDef())
       Flags |= MachineMemOperand::MOStore;
     else
       Flags |= MachineMemOperand::MOLoad;
 
   MachineBasicBlock *MBB = MI->getParent();
   assert(MBB && "foldMemoryOperand needs an inserted instruction");
   MachineFunction &MF = *MBB->getParent();
 
   MachineInstr *NewMI = nullptr;
 
   if (MI->getOpcode() == TargetOpcode::STACKMAP ||
       MI->getOpcode() == TargetOpcode::PATCHPOINT) {
     // Fold stackmap/patchpoint.
     NewMI = foldPatchpoint(MF, MI, Ops, FI, *this);
     if (NewMI)
       MBB->insert(MI, NewMI);
   } else {
     // Ask the target to do the actual folding.
-    NewMI = foldMemoryOperandImpl(MF, MI, Ops, MI, FI);
+    NewMI = foldMemoryOperandImpl(MF, MI, Ops, MI, FI, LIS);
   }
 
   if (NewMI) {
     NewMI->setMemRefs(MI->memoperands_begin(), MI->memoperands_end());
     // Add a memory operand, foldMemoryOperandImpl doesn't do that.
     assert((!(Flags & MachineMemOperand::MOStore) ||
             NewMI->mayStore()) &&
            "Folded a def to a non-store!");

lib/Target/SystemZ/SystemZInstrInfo.h

void loadRegFromStackSlot(MachineBasicBlock &MBB,
                             const TargetRegisterClass *RC,
                             const TargetRegisterInfo *TRI) const override;
   MachineInstr *convertToThreeAddress(MachineFunction::iterator &MFI,
                                       MachineBasicBlock::iterator &MBBI,
                                       LiveVariables *LV) const override;
   MachineInstr *foldMemoryOperandImpl(MachineFunction &MF, MachineInstr *MI,
                                       ArrayRef<unsigned> Ops,
                                       MachineBasicBlock::iterator InsertPt,
-                                      int FrameIndex) const override;
+                                      int FrameIndex,
+                                      LiveIntervals &LIS) const override;
   MachineInstr *foldMemoryOperandImpl(MachineFunction &MF, MachineInstr *MI,
                                       ArrayRef<unsigned> Ops,
                                       MachineBasicBlock::iterator InsertPt,
                                       MachineInstr *LoadMI) const override;
   bool expandPostRAPseudo(MachineBasicBlock::iterator MBBI) const override;
   bool ReverseBranchCondition(SmallVectorImpl<MachineOperand> &Cond) const
     override;
 

lib/Target/SystemZ/SystemZInstrInfo.cpp

 // This file contains the SystemZ implementation of the TargetInstrInfo class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SystemZInstrInfo.h"
 #include "SystemZInstrBuilder.h"
 #include "SystemZTargetMachine.h"
 #include "llvm/CodeGen/LiveVariables.h"
+#include "llvm/CodeGen/LiveIntervalAnalysis.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 
 using namespace llvm;
 
 #define GET_INSTRINFO_CTOR_DTOR
 #define GET_INSTRMAP_INFO
 #include "SystemZGenInstrInfo.inc"
 
if (isRxSBGMask(Imm, And.RegSize, Start, End)) {
       return finishConvertToThreeAddress(MI, MIB, LV);
     }
   }
   return nullptr;
 }
 
 MachineInstr *SystemZInstrInfo::foldMemoryOperandImpl(
     MachineFunction &MF, MachineInstr *MI, ArrayRef<unsigned> Ops,
-    MachineBasicBlock::iterator InsertPt, int FrameIndex) const {
+    MachineBasicBlock::iterator InsertPt, int FrameIndex,
+    LiveIntervals &LIS) const {
+  const TargetRegisterInfo *TRI = MF.getSubtarget().getRegisterInfo();
   const MachineFrameInfo *MFI = MF.getFrameInfo();
   unsigned Size = MFI->getObjectSize(FrameIndex);
   unsigned Opcode = MI->getOpcode();
 
-// XXX This is an introduction of a CC def and is illegal! Reactivate
-// with a check of liveness of CC reg.
-#if 0
   if (Ops.size() == 2 && Ops[0] == 0 && Ops[1] == 1) {
     if ((Opcode == SystemZ::LA || Opcode == SystemZ::LAY) &&
         isInt<8>(MI->getOperand(2).getImm()) &&
         !MI->getOperand(3).getReg()) {
+
+      // Check CC liveness, since new instruction introduces a dead
+      // def of CC.
+      LiveRange &CCLiveRange =
+        LIS.getRegUnit(*MCRegUnitIterator(SystemZ::CC, TRI));
+      SlotIndex MISlot = LIS.getSlotIndexes()->getInstructionIndex(*MI);
+      if (!CCLiveRange.liveAt(MISlot)) {
         // LA(Y) %reg, CONST(%reg) -> AGSI %mem, CONST
         MachineInstr *BuiltMI =
           BuildMI(*InsertPt->getParent(), InsertPt, MI->getDebugLoc(),
-                     get(SystemZ::AGSI))
           .addFrameIndex(FrameIndex)
           .addImm(0)
           .addImm(MI->getOperand(2).getImm());
         BuiltMI->findRegisterDefOperand(SystemZ::CC)->setIsDead(true);
+        CCLiveRange.createDeadDef(MISlot, LIS.getVNInfoAllocator());
         return BuiltMI;
       }
+    }
     return nullptr;
   }
-#endif
 
   // All other cases require a single operand.
   if (Ops.size() != 1)
     return nullptr;
 
   unsigned OpNum = Ops[0];
   assert(Size == MF.getRegInfo()
          .getRegClass(MI->getOperand(OpNum).getReg())->getSize() &&

test/CodeGen/SystemZ/int-add-12.ll

; CHECK: br %r14
   %add = add i64 %val, 1
   store i64 %add, i64 *%ptr
   ret void
 }
 
 ; Check that adding 127 to a spilled value can use AGSI.
 define void @f11(i64 *%ptr, i32 %sel) {
 ; CHECK-LABEL: f11:
-; _CHECK: agsi {{[0-9]+}}(%r15), 127
+; CHECK: agsi {{[0-9]+}}(%r15), 127
 ; CHECK: br %r14
 entry:
   %val0 = load volatile i64 , i64 *%ptr
   %val1 = load volatile i64 , i64 *%ptr
   %val2 = load volatile i64 , i64 *%ptr
   %val3 = load volatile i64 , i64 *%ptr
   %val4 = load volatile i64 , i64 *%ptr
   %val5 = load volatile i64 , i64 *%ptr