[PPC] override the base implementatiosn of areLoadsFromSameBasePtr and shouldScheduleLoadsNear for PowerPC

[sfertile]

Summary

Implements overrides for the 'areLoadsFromSameBasePtr' function and 'shouldScheduleLoadsNear' function. Implementing these allows the pre-ra-scheduler to identify loads that are relatively close together and glue them together for scheduling purposes. The patch adds 2 back-end options allowing tuning of the number of vector/scalar loads to try to cluster together.

[sfertile]

Noticed spelling mistake and ended up simplifying the loop in getChainOperand as part of the cleanup.

[hfinkel]

Interesting; I didn't realize that our SDAG-based scheduling actually did anything at this point (i.e. we're just using source-order scheduling) - The MI scheduler is where the real action happens, and that has a different clustering API. You should look at TII->shouldClusterMemOps and the other things used by BaseMemOpClusterMutation in MachineScheduler.cpp. I think we should only implement these older interfaces if there's some fundamental advantage to doing so. Otherwise, I'd focus on what the MI scheduler uses.

lib/Target/PowerPC/PPCInstrInfo.cpp

1941

Space after 'if' (and so on for other 'if', 'while', etc. below).

2026

We should synchronize this with PPCTTIImpl::getCacheLineSize().

[sfertile]

Thanks for the feedback. I was looking at cleaning up the loads/stores from memcpy expansions and this worked pretty good for 64 bytes and under. Looking at a 128 byte expansion in BoringSSL was showing a number of limitations with this approach though. I'll look into the things you mention in MachineScheduler to see if I can get the behavior I'm looking for there.

[nemanjai]

Is this patch superseded by https://reviews.llvm.org/D29779? If so, please abandon this patch so it moves out of the reviewers' list.

[sfertile]

After some investigation, I don't think LoadCluster/StoreCluster DAG mutations will provide the clustering I was looking to implement. Even with that I don't think we want to proceed with this patch either so I will abandon it as Nemanja suggests.

lib/Target/PowerPC/PPCInstrInfo.h

unsigned isStoreToStackSlot(const MachineInstr &MI,
                               int &FrameIndex) const override;
 
   bool findCommutedOpIndices(MachineInstr &MI, unsigned &SrcOpIdx1,
                              unsigned &SrcOpIdx2) const override;
 
   void insertNoop(MachineBasicBlock &MBB,
                   MachineBasicBlock::iterator MI) const override;
 
+  /// This is used by the pre-regalloc scheduler to determine if two loads are
+  /// loading from the same base address. It should only return true if the base
+  /// pointers are the same and the only differences between the two addresses
+  /// are the offset. It also returns the offsets by reference.
+  bool areLoadsFromSameBasePtr(SDNode *Load1, SDNode *Load2,
+                               int64_t &Offset1,
+                               int64_t &Offset2) const override;
+
+  /// shouldScheduleLoadsNear - This is a used by the pre-regalloc scheduler to
+  /// determine (in conjunction with areLoadsFromSameBasePtr) if two loads should
+  /// be scheduled togther. On some targets if two loads are loading from
+  /// addresses in the same cache line, it's better if they are scheduled
+  /// together. This function takes two integers that represent the load offsets
+  /// from the common base address. It returns true if it decides it's desirable
+  /// to schedule the two loads together. "NumLoads" is the number of loads that
+  /// have already been scheduled after Load1.
+  bool shouldScheduleLoadsNear(SDNode *Load1, SDNode *Load2,
+                               int64_t Offset1, int64_t Offset2,
+                               unsigned NumLoads) const override;
+
 
   // Branch analysis.
   bool analyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
                      MachineBasicBlock *&FBB,
                      SmallVectorImpl<MachineOperand> &Cond,
                      bool AllowModify) const override;
   unsigned removeBranch(MachineBasicBlock &MBB,
                         int *BytesRemoved = nullptr) const override;

lib/Target/PowerPC/PPCInstrInfo.cpp

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
+static cl::opt<unsigned> VectorLoadClusterCount("vec-load-clustering", cl::Hidden, cl::init(3));
+static cl::opt<unsigned> ScalarLoadClusterCount("scalar-load-clustering", cl::Hidden, cl::init(3));
+
 #define DEBUG_TYPE "ppc-instr-info"
 
 #define GET_INSTRMAP_INFO
 #define GET_INSTRINFO_CTOR_DTOR
 #include "PPCGenInstrInfo.inc"
 
 static cl::
 opt<bool> DisableCTRLoopAnal("disable-ppc-ctrloop-analysis", cl::Hidden,

 }
 
 const TargetRegisterClass *
 PPCInstrInfo::updatedRC(const TargetRegisterClass *RC) const {
   if (Subtarget.hasVSX() && RC == &PPC::VRRCRegClass)
     return &PPC::VSRCRegClass;
   return RC;
 }
+
+
+static MachineMemOperand* extractMemOp(MachineSDNode *Load) {
+  MachineMemOperand **IMemOp = Load->memoperands_begin();
+  MachineMemOperand* MMO = nullptr;
+
+  if(IMemOp) {
+    MMO = *IMemOp;
+    assert(++IMemOp == Load->memoperands_end() &&
+           "Expect a single memory operand in a load");
+  }
+
+  return MMO;
+}
+
+// Some machine instructions may have both mayLoad and mayStore flags set.
+// These instructions are lowered from intrinsics that don't actually
+// touch memory, but can not have the IntrNoMem flags becuase it needs to be
+// inserted into a chain due to their side-effects. These side-effect only
+// intrinsics will have both mayLoad and mayStore flags set.
+static bool mayActuallyLoad(MCInstrDesc Desc) {
+  return Desc.mayLoad() && !Desc.mayStore();
+}
+
+// gets the chain operand from an SDNode.
+static SDValue getChainOperand(SDNode *Node) {
+  // Loop past any glue nodes.
+  unsigned OpIndex  = Node->getNumOperands();
+  while(OpIndex && Node->getOperand(OpIndex - 1).getValueType() == MVT::Glue) {
+    --OpIndex;
+  }
+
+  assert(OpIndex && "expected at least one Operand after Glue operands!");
+  // Opindex is either the index of the last glue node, or the totoal number of
+  // operands if there are no glue nodes. The chain operand must be the previous
+  // operand.
+  SDValue ChainOp = Node->getOperand(--OpIndex);
+  assert(ChainOp.getValueType() == MVT::Other &&
+         "Expected Chain Operand on mayLoad MachineSDNode!");
+  return ChainOp;
+}
+
+bool PPCInstrInfo::areLoadsFromSameBasePtr(SDNode *Load1, SDNode *Load2,
+                                    int64_t &Offset1, int64_t &Offset2) const {
+
+  // Only interested in MachineSDNodes
+  if(!Load1->isMachineOpcode() || !Load2->isMachineOpcode()) {
+    return false;
+  }
+
+  const MCInstrDesc &MCIDesc1 = get(Load1->getMachineOpcode());
+  const MCInstrDesc &MCIDesc2 = get(Load2->getMachineOpcode());
+  // Only interested in 'real' loads.
+  if(!mayActuallyLoad(MCIDesc1) || MCIDesc1.isPseudo() ||
+     !mayActuallyLoad(MCIDesc2) || MCIDesc2.isPseudo()) {
+    return false;
+  }
+
+  // only interested in Loads in the same chain.
+  if(getChainOperand(Load1) != getChainOperand(Load2)) {
+    return false;
+  }
+
+  // Get the memory operands
+  MachineSDNode *MachineLoad1 = dyn_cast<MachineSDNode>(Load1);
+  MachineSDNode *MachineLoad2 = dyn_cast<MachineSDNode>(Load2);
+  assert(MachineLoad1 && MachineLoad1);
+  MachineMemOperand *MemOp1 = extractMemOp(MachineLoad1);
+  MachineMemOperand *MemOp2 = extractMemOp(MachineLoad2);
+
+  // Not every load will have its MMO properly set. For example the loads
+  // created from intrinsic calls may not have them set.
+  if(!MemOp1 || !MemOp2)
+     return false;
+
+  // Check that the memory ops use the same base value
+  if(MemOp1->getValue() == MemOp2->getValue()) {
+    Offset1 = MemOp1->getOffset();
+    Offset2 = MemOp2->getOffset();
+    return true;
+  }
+
+  // Loads are off different base values.
+  return false;
+}
+
+bool PPCInstrInfo::shouldScheduleLoadsNear(SDNode *Load1, SDNode *Load2,
+                             int64_t Offset1, int64_t Offset2,
+                             unsigned NumLoads) const  {
+  assert(Offset2 > Offset1 && "Offset2 must larger then Offset1!");
+
+  // Check that the loads are whithin a cacheline of each other.
+  if(Offset2 - Offset1 > 128)
+     return false;
+
+  EVT VT = Load1->getValueType(0);
+  if (VT.isVector()) {
+    return NumLoads < VectorLoadClusterCount ;
+  }
+
+  return NumLoads < ScalarLoadClusterCount;
+}

test/CodeGen/PowerPC/fp128-bitcast-after-operation.ll

 ; PPC64-P8-DAG: li [[MASK_REG:[0-9]+]], 1
 ; PPC64-P8-DAG: sldi [[SHIFT_REG:[0-9]+]], [[MASK_REG]], 63
 ; PPC64-P8: and [[FLIP_BIT:[0-9]+]], [[HI]], [[SHIFT_REG]]
 ; PPC64-P8-DAG: xor 3, [[HI]], [[FLIP_BIT]]
 ; PPC64-P8-DAG: xor 4, [[LO]], [[FLIP_BIT]]
 ; PPC64-P8: blr
 
 ; PPC32-DAG: stfd 1, 24(1)
-; PPC32-DAG: stfd 2, 16(1)
+; PPC32: nop
 ; PPC32: nop
 ; PPC32-DAG: lwz [[HI0:[0-9]+]], 24(1)
-; PPC32-DAG: lwz [[LO0:[0-9]+]], 16(1)
 ; PPC32-DAG: lwz [[HI1:[0-9]+]], 28(1)
+; PPC32-DAG: stfd 2, 16(1)
+; PPC32-DAG: lwz [[LO0:[0-9]+]], 16(1)
 ; PPC32-DAG: lwz [[LO1:[0-9]+]], 20(1)
 ; PPC32: rlwinm [[FLIP_BIT:[0-9]+]], [[HI0]], 0, 0, 0
 ; PPC32-DAG: xor [[HI0]], [[HI0]], [[FLIP_BIT]]
 ; PPC32-DAG: xor [[LO0]], [[LO0]], [[FLIP_BIT]]
 ; PPC32: blr
   %0 = tail call ppc_fp128 @llvm.fabs.ppcf128(ppc_fp128 %x)
   %1 = bitcast ppc_fp128 %0 to i128
   ret i128 %1

test/CodeGen/PowerPC/jaggedstructs.ll

 ; CHECK: lwz {{[0-9]+}}, 171(1)
 ; CHECK: stb {{[0-9]+}}, 63(1)
 ; CHECK: stw {{[0-9]+}}, 59(1)
 ; CHECK: lhz {{[0-9]+}}, 182(1)
 ; CHECK: lwz {{[0-9]+}}, 178(1)
 ; CHECK: sth {{[0-9]+}}, 70(1)
 ; CHECK: stw {{[0-9]+}}, 66(1)
 ; CHECK: lbz {{[0-9]+}}, 191(1)
-; CHECK: lhz {{[0-9]+}}, 189(1)
 ; CHECK: lwz {{[0-9]+}}, 185(1)
-; CHECK: stb {{[0-9]+}}, 79(1)
+; CHECK: lhz {{[0-9]+}}, 189(1)
 ; CHECK: sth {{[0-9]+}}, 77(1)
+; CHECK: stb {{[0-9]+}}, 79(1)
 ; CHECK: stw {{[0-9]+}}, 73(1)
 ; CHECK: ld 6, 72(1)
 ; CHECK: ld 5, 64(1)
 ; CHECK: ld 4, 56(1)
 ; CHECK: ld 3, 48(1)
 
 declare void @check(%struct.S3* byval, %struct.S5* byval, %struct.S6* byval, %struct.S7* byval)

test/CodeGen/PowerPC/memcpy-vec.ll

entry:
   %0 = bitcast double* %x to i8*
   %1 = bitcast double* %y to i8*
   tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* %0, i8* %1, i64 32, i32 8, i1 false)
   ret void
 
 ; PWR7-LABEL: @foo1
 ; PWR7-NOT: bl memcpy
 ; PWR7-DAG: li [[OFFSET:[0-9]+]], 16
-; PWR7-DAG: lxvd2x [[TMP0:[0-9]+]], 4, [[OFFSET]]
+; PWR7-DAG: lxvd2x [[TMP0:[0-9]+]], 0, 4
+; PWR7-DAG: lxvd2x [[TMP1:[0-9]+]], 4, [[OFFSET]]
+; PWR7-DAG: stxvd2x [[TMP1]], 3, [[OFFSET]]
 ; PWR7-DAG: stxvd2x [[TMP0]], 0, 3
-; PWR7-DAG: lxvd2x [[TMP1:[0-9]+]], 0, 4
-; PWR7-DAG: stxvd2x [[TMP1]], 0, 3
 ; PWR7: blr
 
 ; PWR8-LABEL: @foo1
 ; PWR8: lxvw4x
 ; PWR8: stxvw4x
 ; PWR8: blr
 
 ; A2Q-LABEL: @foo1

test/CodeGen/PowerPC/ppc32-vacopy.ll

entry:
   call void @llvm.va_copy(i8* %3, i8* %2)
 
   ret void
 }
 ; CHECK: test_vacopy:
 ; CHECK: lwz [[REG1:[0-9]+]], {{.*}}
 ; CHECK: lwz [[REG2:[0-9]+]], {{.*}}
 ; CHECK: lwz [[REG3:[0-9]+]], {{.*}}
-; CHECK: stw [[REG1]], {{.*}}
-; CHECK: stw [[REG2]], {{.*}}
 ; CHECK: stw [[REG3]], {{.*}}
+; CHECK: stw [[REG2]], {{.*}}
+; CHECK: stw [[REG1]], {{.*}}

test/CodeGen/PowerPC/ppcf128-endian.ll

 define void @caller() {
 entry:
   %0 = load ppc_fp128, ppc_fp128* @g, align 16
   call void @test(ppc_fp128 %0)
   ret void
 }
 ; CHECK: @caller
 ; CHECK: ld [[REG:[0-9]+]], .LC
-; CHECK: lfd 2, 8([[REG]])
 ; CHECK: lfd 1, 0([[REG]])
+; CHECK: lfd 2, 8([[REG]])
 ; CHECK: bl test
 
 declare void @test(ppc_fp128)
 
 define void @caller_const() {
 entry:
   call void @test(ppc_fp128 0xM3FF00000000000000000000000000000)
   ret void