Change default implementation of doMemOpsHaveSameBasePtr

[rampitec]

Summary

This changes default implementation of the doMemOpsHaveSameBasePtr
target callback used for load/store clustering as requested in the
D37698.

The implementation is switched to the old one for the AArch64
target as it seems to expect same base registers with ordered
offsets for further combining purposes.

The change is based on the D37698.

[hfinkel]

Please see my comments in D37698. If the primary purpose of this on AArch64 is to enable the AArch64LoadStoreOptimizer, I'm not sure that this makes sense.

[rampitec]

In D37755#869341, @hfinkel wrote:

Please see my comments in D37698. If the primary purpose of this on AArch64 is to enable the AArch64LoadStoreOptimizer, I'm not sure that this makes sense.

Right. After I did it and run testing I have realized only to targets in the tree use these mutations, and AArch64 needs exactly current implementation. So I would keep it AMD specific untill there are other users.

include/llvm/Target/TargetInstrInfo.h

public:
   }
 
   /// If the instruction is an increment of a constant value, return the amount.
   virtual bool getIncrementValue(const MachineInstr &MI, int &Value) const {
     return false;
   }
 
   /// Used by MachineScheduler to determine if it should attempt to cluster
-  /// these memory operations. Implementation of this callback is needed
-  /// when an instruction requires a fully calculated address and does not
-  /// have separate base register and offset operands. If it does have
-  /// separate offset field the getMemOpBaseRegImmOfs should suffice.
+  /// these memory operations. Arguments are two candidate instructions
+  /// and base registers as returned by the getMemOpBaseRegImmOfs.
   virtual bool doMemOpsHaveSameBasePtr(const MachineInstr &MI1,
                                        unsigned BaseReg1,
                                        const MachineInstr &MI2,
-                                       unsigned BaseReg2) const {
-    return BaseReg1 == BaseReg2;
-  }
+                                       unsigned BaseReg2) const;
 
   /// Returns true if the two given memory operations should be scheduled
   /// adjacent. Note that you have to add:
   ///   DAG->addMutation(createLoadClusterDAGMutation(DAG->TII, DAG->TRI));
   /// or
   ///   DAG->addMutation(createStoreClusterDAGMutation(DAG->TII, DAG->TRI));
   /// to TargetPassConfig::createMachineScheduler() to have an effect.
   virtual bool shouldClusterMemOps(MachineInstr &FirstLdSt,

lib/CodeGen/TargetInstrInfo.cpp

 //===-- TargetInstrInfo.cpp - Target Instruction Information --------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the TargetInstrInfo class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Target/TargetInstrInfo.h"
+#include "llvm/Analysis/ValueTracking.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineMemOperand.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/PseudoSourceValue.h"
 #include "llvm/CodeGen/ScoreboardHazardRecognizer.h"
 #include "llvm/CodeGen/StackMaps.h"
 #include "llvm/CodeGen/TargetSchedule.h"

 // 3. The instruction must have a reassociable sibling.
 bool TargetInstrInfo::isReassociationCandidate(const MachineInstr &Inst,
                                                bool &Commuted) const {
   return isAssociativeAndCommutative(Inst) &&
          hasReassociableOperands(Inst, Inst.getParent()) &&
          hasReassociableSibling(Inst, Commuted);
 }
 
+/// Used by MachineScheduler to determine if it should attempt to cluster
+/// these memory operations. Arguments are two candidate instructions
+/// and base registers as returned by the getMemOpBaseRegImmOfs.
+bool TargetInstrInfo::doMemOpsHaveSameBasePtr(const MachineInstr &MI1,
+                                              unsigned BaseReg1,
+                                              const MachineInstr &MI2,
+                                              unsigned BaseReg2) const {
+  if (BaseReg1 == BaseReg2)
+    return true;
+
+  if (!MI1.hasOneMemOperand() || !MI2.hasOneMemOperand())
+    return false;
+
+  auto MO1 = *MI1.memoperands_begin();
+  auto MO2 = *MI2.memoperands_begin();
+  if (MO1->getAddrSpace() != MO2->getAddrSpace())
+    return false;
+
+  auto Base1 = MO1->getValue();
+  auto Base2 = MO2->getValue();
+  if (!Base1 || !Base2)
+    return false;
+  const MachineFunction &MF = *MI1.getParent()->getParent();
+  const DataLayout &DL = MF.getFunction()->getParent()->getDataLayout();
+  Base1 = GetUnderlyingObject(Base1, DL);
+  Base2 = GetUnderlyingObject(Base1, DL);
+
+  if (isa<UndefValue>(Base1) || isa<UndefValue>(Base2))
+    return false;
+
+  return Base1 == Base2;
+}
+
 // The concept of the reassociation pass is that these operations can benefit
 // from this kind of transformation:
 //
 // A = ? op ?
 // B = A op X (Prev)
 // C = B op Y (Root)
 // -->
 // A = ? op ?

lib/Target/AArch64/AArch64InstrInfo.h

public:
 
   /// \brief Returns true if opcode \p Opc is a memory operation. If it is, set
   /// \p Scale, \p Width, \p MinOffset, and \p MaxOffset accordingly.
   ///
   /// For unscaled instructions, \p Scale is set to 1.
   bool getMemOpInfo(unsigned Opcode, unsigned &Scale, unsigned &Width,
                     int64_t &MinOffset, int64_t &MaxOffset) const;
 
+  bool doMemOpsHaveSameBasePtr(const MachineInstr &SU1, unsigned BaseReg1,
+                               const MachineInstr &SU2, unsigned BaseReg2)
+                              const final {
+    return BaseReg1 == BaseReg2;
+  }
+
   bool shouldClusterMemOps(MachineInstr &FirstLdSt, MachineInstr &SecondLdSt,
                            unsigned NumLoads) const override;
 
   void copyPhysRegTuple(MachineBasicBlock &MBB, MachineBasicBlock::iterator I,
                         const DebugLoc &DL, unsigned DestReg, unsigned SrcReg,
                         bool KillSrc, unsigned Opcode,
                         llvm::ArrayRef<unsigned> Indices) const;
   void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator I,

lib/Target/AMDGPU/SIInstrInfo.h

public:
   bool areLoadsFromSameBasePtr(SDNode *Load1, SDNode *Load2,
                                int64_t &Offset1,
                                int64_t &Offset2) const override;
 
   bool getMemOpBaseRegImmOfs(MachineInstr &LdSt, unsigned &BaseReg,
                              int64_t &Offset,
                              const TargetRegisterInfo *TRI) const final;
 
-  bool doMemOpsHaveSameBasePtr(const MachineInstr &SU1, unsigned BaseReg1,
-                               const MachineInstr &SU2, unsigned BaseReg2)
-                              const final;
-
   bool shouldClusterMemOps(MachineInstr &FirstLdSt, MachineInstr &SecondLdSt,
                            unsigned NumLoads) const final;
 
   void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
                    const DebugLoc &DL, unsigned DestReg, unsigned SrcReg,
                    bool KillSrc) const override;
 
   unsigned calculateLDSSpillAddress(MachineBasicBlock &MBB, MachineInstr &MI,

lib/Target/AMDGPU/SIInstrInfo.cpp

 #include "Utils/AMDGPUBaseInfo.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/MemoryLocation.h"
-#include "llvm/Analysis/ValueTracking.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineInstrBundle.h"
 #include "llvm/CodeGen/MachineMemOperand.h"
 #include "llvm/CodeGen/MachineOperand.h"
if (isFLAT(LdSt)) {
 
     Offset = getNamedOperand(LdSt, AMDGPU::OpName::offset)->getImm();
     return true;
   }
 
   return false;
 }
 
-bool SIInstrInfo::doMemOpsHaveSameBasePtr(const MachineInstr &MI1,
-                                          unsigned BaseReg1,
-                                          const MachineInstr &MI2,
-                                          unsigned BaseReg2) const {
-  if (BaseReg1 == BaseReg2)
-    return true;
-
-  if (!MI1.hasOneMemOperand() || !MI2.hasOneMemOperand())
-    return false;
-
-  auto MO1 = *MI1.memoperands_begin();
-  auto MO2 = *MI2.memoperands_begin();
-  if (MO1->getAddrSpace() != MO2->getAddrSpace())
-    return false;
-
-  auto Base1 = MO1->getValue();
-  auto Base2 = MO2->getValue();
-  if (!Base1 || !Base2)
-    return false;
-  const MachineFunction &MF = *MI1.getParent()->getParent();
-  const DataLayout &DL = MF.getFunction()->getParent()->getDataLayout();
-  Base1 = GetUnderlyingObject(Base1, DL);
-  Base2 = GetUnderlyingObject(Base1, DL);
-
-  if (isa<UndefValue>(Base1) || isa<UndefValue>(Base2))
-    return false;
-
-  return Base1 == Base2;
-}
-
 bool SIInstrInfo::shouldClusterMemOps(MachineInstr &FirstLdSt,
                                       MachineInstr &SecondLdSt,
                                       unsigned NumLoads) const {
   const MachineOperand *FirstDst = nullptr;
   const MachineOperand *SecondDst = nullptr;
 
   if ((isMUBUF(FirstLdSt) && isMUBUF(SecondLdSt)) ||
       (isMTBUF(FirstLdSt) && isMTBUF(SecondLdSt)) ||