[PPC] - Add LoadCluster/StoreCluster DAG mutations to the MachineScheduler

[inouehrs]

lib/Target/PowerPC/PPCInstrInfo.cpp

2016

Here, do you mean cache line size?
Some PPC processors use a shorter cache line size (e.g. L1$ of BG/Q or embedded PPC), so maybe better to avoid hardcoding a value.
(But, TTI->getCacheLineSize() always returns 64, since it is only used on BG/Q so far.)

2127

Ditto.

[nemanjai]

Please be a bit more careful to ensure that comments are complete sentences with the expected punctuation, etc. Also, if your editor doesn't automatically format your code, consider running the patch through clang-format-diff.

lib/Target/PowerPC/PPCInstrInfo.cpp

1937

What is the use of the string parameter?

1940

Naming convention.

1950

"...are instructions are..." -> "... instructions are..."

1953

Please fix the formatting in this comment. I believe we should send out an RFC on llvm-dev to provide a new property for intrinsics that specifies that they have side-effects but do not touch memory. We have such a flag for instructions, I think it makes sense to have it for intrinsics as well. If we do that, I believe this comment should include a FIXME to remove this function once such a property exists and is used correctly in the PPC back end in all instances where it should be used.

1957

Why can't SDNode::getGluedNode() be used for this purpose and eliminate the need for this function?

1959

Is the operand you're after not guaranteed to be the very last operand?
In any case, it seems more natural to loop over the operands in reverse order, terminating before the first - and return the Glue operand when one is found. If the loop terminates, assert and return SDValue().

1965

Why is this needed? It seems that the above loop has terminated either when it has gone over all the operands (caught by previous assert) or when it found an operand whose type is MVT::Glue (whose type you check in this assert again). I think the weird structure of this function makes the logic more difficult to reason about than it needs to be.

1966

Keep in mind that you'll be returning something that you probably don't want to return (i.e. what you would assert on) in a no-asserts build.

1992

Text for the assert.

2001

tlat -> that

2015

Why?

2016

I agree. If we mean cache line size, we should check the cache line size. If we don't have accurate information about the cache line size for the common processors in the TTI implementation, we should update the implementation.

2037

This assert is unnecessary. Already exists in getImm().

2099

I think you may have physical registers at this point for TOC based loads (i.e. X2/R2 may already be part of the instruction at this point). It would make sense to look into whether this is the case and if it is, we should handle physical registers. Other than that, I don't see any compelling need to handle physical registers here.

2102

I am probably missing something here so it's a good indication that a detailed comment is in order...
What I mean is that it appears at first glance that if we have a load-hit-store situation where the store is followed by a load from the exact memory location, we'd happily cluster them together here. Presumably there's something after this (or before?) that will prevent this, but I think it would be good to document that here.

2103

This line looks way too long.

2106

A comment as to why we're exiting here.

lib/Target/PowerPC/PPCInstrInfo.h

52

Please refrain from unrelated formatting changes.

183

"... a used..." -> "... used ..."

204

Sentence fragment in comment, please revise.

[sfertile]

lib/Target/PowerPC/PPCInstrInfo.cpp

1937

I was using the string argument for some debugging and left it in by accident. I'll clean it up.

2016

Yeah this is the cache-line size. I'll update it the TTI implementation like you guys suggest as part of this patch

lib/Target/PowerPC/PPCInstrInfo.h

                   MachineBasicBlock::iterator MI) const override;
 
 
+
+  /// Get the base register and byte offset of an instruction that reads/writes
+  /// memory.
+  virtual bool getMemOpBaseRegImmOfs(MachineInstr &MemOp, unsigned &BaseReg,
+                                     int64_t &Offset,
+                                     const TargetRegisterInfo *TRI) const override;
+
+  /// Returns true if the two given memory operations should be scheduled
+  /// adjacent. Called from the LoadCluster/StoreCluster DAG mutation passes.
+  /// to TargetPassConfig::createMachineScheduler() to have an effect.
+  virtual bool shouldClusterMemOps(MachineInstr &FirstLdSt,
+                                   MachineInstr &SecondLdSt,
+                                   unsigned NumLoads) const override;
+
   // Branch analysis.
   bool analyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
                      MachineBasicBlock *&FBB,

lib/Target/PowerPC/PPCInstrInfo.cpp

     return &PPC::VSRCRegClass;
   return RC;
 }
+
+static bool tryExtractImm(MachineInstr *MI, int64_t &Offset) {
+  switch (MI->getOpcode()) {
+  default:
+    return false;
+  case PPC::LI8:
+  case PPC::LI:
+    assert(MI->getOperand(1).isImm());
+    Offset = MI->getOperand(1).getImm();
+    return true;
+  }
+}
+
+bool PPCInstrInfo::getMemOpBaseRegImmOfs(MachineInstr &MemOp, unsigned &BaseReg,
+                                         int64_t &Offset,
+                                         const TargetRegisterInfo *TRI) const {
+
+  const MCInstrDesc &InstrDesc = get(MemOp.getOpcode());
+
+  // mayLoad and mayStore means its a MachineInstr with side-effects rather then
+  // an actual memory operation.
+  if (InstrDesc.mayLoad() && InstrDesc.mayStore()) {
+    return false;
+  }
+
+  // expect at least 3 (explicit) operands.
+  if (MemOp.getNumExplicitOperands() < 3) {
+    return false;
+  }
+
+  MachineOperand &Mo1 = MemOp.getOperand(1);
+  MachineOperand &Mo2 = MemOp.getOperand(2);
+
+  // Check for 'BaseAddr + imm' and 'imm + BaseAddr'.
+  if (Mo1.isReg() && Mo2.isImm()) {
+    BaseReg = Mo1.getReg();
+    Offset = Mo2.getImm();
+    return true;
+  } else if (Mo2.isReg() && Mo1.isImm()) {
+    BaseReg = Mo2.getReg();
+    Offset = Mo1.getImm();
+    return true;
+  }
+
+  if (!(Mo1.isReg() && Mo2.isReg())) {
+    return false;
+  }
+
+  // check for 'r0 + BaseAddr'
+  if (Mo1.getReg() == PPC::ZERO8 || Mo1.getReg() == PPC::ZERO) {
+    BaseReg = Mo2.getReg();
+    Offset = 0;
+    return true;
+  }
+
+  BaseReg = Mo1.getReg();
+  MachineRegisterInfo &MRI = MemOp.getParent()->getParent()->getRegInfo();
+
+  // Try to extract an immediate from the register definition.
+  if (TargetRegisterInfo::isVirtualRegister(Mo2.getReg())) {
+    // May have more then one def
+    if (!MRI.hasOneDef(Mo2.getReg())) {
+      return false;
+    }
+
+    MachineInstr *MI = MRI.getVRegDef(Mo2.getReg());
+    return tryExtractImm(MI, Offset);
+  }
+
+  // TODO Do I need to handle physical registers at this point?
+  return false;
+}
+
+bool PPCInstrInfo::shouldClusterMemOps(MachineInstr &FirstLdSt,
+                                       MachineInstr &SecondLdSt,
+                                       unsigned NumLoads) const {
+
+  if (FirstLdSt.hasOrderedMemoryRef() || SecondLdSt.hasOrderedMemoryRef()) {
+    return false;
+  }
+
+  const TargetRegisterInfo *TRI = &getRegisterInfo();
+  unsigned FirstBaseReg, SecondBaseReg;
+  int64_t FirstOffset, SecondOffset;
+
+  if (!getMemOpBaseRegImmOfs(FirstLdSt, FirstBaseReg, FirstOffset, TRI) ||
+      !getMemOpBaseRegImmOfs(SecondLdSt, SecondBaseReg, SecondOffset, TRI)) {
+    return false;
+  }
+
+  // Only cluster loads from the same base address
+  if (FirstBaseReg != SecondBaseReg)
+    return false;
+
+  int64_t OffsetDiff = FirstOffset > SecondOffset ? FirstOffset - SecondOffset
+                                                  : SecondOffset - FirstOffset;
+
+  if (OffsetDiff > 128) {
+    return false;
+  }
+
+  return true;
+}

lib/Target/PowerPC/PPCTargetMachine.cpp

 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
+#include "llvm/CodeGen/MachineScheduler.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"

     return getTM<PPCTargetMachine>();
   }
 
+  ScheduleDAGInstrs *
+  createMachineScheduler(MachineSchedContext *C) const override {
+    // TODO: Create the default for non-P8 Targets.
+    // ScheduleDAGInstrs *DAG =  TargetPassConfig::createMachineScheduler(C);
+    ScheduleDAGMILive *DAG = createGenericSchedLive(C);
+    DAG->addMutation(createLoadClusterDAGMutation(DAG->TII, DAG->TRI));
+    DAG->addMutation(createStoreClusterDAGMutation(DAG->TII, DAG->TRI));
+
+    return DAG;
+  }
+
   void addIRPasses() override;
   bool addPreISel() override;
   bool addILPOpts() override;

test/CodeGen/PowerPC/cluster.ll

+; RUN: llc -verify-machineinstrs -mattr=-altivec < %s | FileCheck %s -check-prefix=CHECK
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 < %s | FileCheck %s -check-prefix=PWR9
+
+target datalayout = "e-m:e-i64:64-n32:64"
+target triple = "powerpc64le-unknown-linux-gnu"
+
+define void @foo(i64* noalias nocapture readonly %src, i64* noalias nocapture %dst) local_unnamed_addr #0 {
+entry:
+; CHECK-LABEL: @foo
+; CHECK: ld [[REG1:[0-9]+]], 0([[BASE1:[0-9]+]])
+; CHECK-NEXT: ld [[REG2:[0-9]+]], 8([[BASE1]])
+; CHECK-NEXT: ld [[REG3:[0-9]+]], 16([[BASE1]])
+; CHECK-NEXT: std [[REG1]], 0([[BASE2:[0-9]+]])
+; CHECK-NEXT: std [[REG2]], 8([[BASE2]])
+; CHECK-NEXT: std [[REG3]], 16([[BASE2]])
+  %0 = load i64, i64* %src, align 8, !tbaa !2
+  store i64 %0, i64* %dst, align 8, !tbaa !2
+  %arrayidx2 = getelementptr inbounds i64, i64* %src, i64 1
+  %1 = load i64, i64* %arrayidx2, align 8, !tbaa !2
+  %arrayidx3 = getelementptr inbounds i64, i64* %dst, i64 1
+  store i64 %1, i64* %arrayidx3, align 8, !tbaa !2
+  %arrayidx4 = getelementptr inbounds i64, i64* %src, i64 2
+  %2 = load i64, i64* %arrayidx4, align 8, !tbaa !2
+  %arrayidx5 = getelementptr inbounds i64, i64* %dst, i64 2
+  store i64 %2, i64* %arrayidx5, align 8, !tbaa !2
+  ret void
+}
+
+define void @foo2(i64* noalias nocapture readonly %src, i64* noalias nocapture %dst) local_unnamed_addr #0 {
+entry:
+; PWR9-LABEL: @foo2
+; PWR9: li [[OFF1:[0-9]+]], 16
+; PWR9-NEXT: li [[OFF2:[0-9]+]], 32
+; PWR9-NEXT: li [[OFF3:[0-9]+]], 48
+; PWR9-NEXT: lxvx [[REG:[0-9]+]], 0, [[BASE1:[0-9]+]]
+; PWR9-NEXT: stxvx [[REG]], 0, [[BASE2:[0-9]+]]
+; PWR9-DAG: lxvx [[REG1:[0-9]+]], [[BASE1]], [[OFF1]]
+; PWR9-NEXT: lxvx [[REG2:[0-9]+]], [[BASE1]], [[OFF2]]
+; PWR9-NEXT: lxvx [[REG3:[0-9]+]], [[BASE1]], [[OFF3]]
+; PWR9-NEXT: stxvx [[REG1]], [[BASE2]], [[OFF1]]
+; PWR9-NEXT: stxvx [[REG2]], [[BASE2]], [[OFF2]]
+; PWR9-NEXT: stxvx [[REG3]], [[BASE2]], [[OFF3]]
+
+  %0 = bitcast i64* %src to <2 x i64>*
+  %1 = load <2 x i64>, <2 x i64>* %0, align 8, !tbaa !2
+  %2 = bitcast i64* %dst to <2 x i64>*
+  store <2 x i64> %1, <2 x i64>* %2, align 8, !tbaa !2
+  %arrayidx4 = getelementptr inbounds i64, i64* %src, i64 2
+  %arrayidx5 = getelementptr inbounds i64, i64* %dst, i64 2
+  %3 = bitcast i64* %arrayidx4 to <2 x i64>*
+  %4 = load <2 x i64>, <2 x i64>* %3, align 8, !tbaa !2
+  %5 = bitcast i64* %arrayidx5 to <2 x i64>*
+  store <2 x i64> %4, <2 x i64>* %5, align 8, !tbaa !2
+  %arrayidx8 = getelementptr inbounds i64, i64* %src, i64 4
+  %arrayidx9 = getelementptr inbounds i64, i64* %dst, i64 4
+  %6 = bitcast i64* %arrayidx8 to <2 x i64>*
+  %7 = load <2 x i64>, <2 x i64>* %6, align 8, !tbaa !2
+  %8 = bitcast i64* %arrayidx9 to <2 x i64>*
+  store <2 x i64> %7, <2 x i64>* %8, align 8, !tbaa !2
+  %arrayidx12 = getelementptr inbounds i64, i64* %src, i64 6
+  %arrayidx13 = getelementptr inbounds i64, i64* %dst, i64 6
+  %9 = bitcast i64* %arrayidx12 to <2 x i64>*
+  %10 = load <2 x i64>, <2 x i64>* %9, align 8, !tbaa !2
+  %11 = bitcast i64* %arrayidx13 to <2 x i64>*
+  store <2 x i64> %10, <2 x i64>* %11, align 8, !tbaa !2
+  ret void
+}
+
+attributes #0 = { norecurse nounwind }
+
+!2 = !{!3, !3, i64 0}
+!3 = !{!"long long", !4, i64 0}
+!4 = !{!"omnipotent char", !5, i64 0}
+!5 = !{!"Simple C/C++ TBAA"}

test/CodeGen/PowerPC/ppcf128-endian.ll

 }
 ; 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)