[TargetInstrInfo] Add new hook: getMemOpBaseRegImmOfs.

[sanjoy]

Summary

NFC: no-one uses getMemOpBaseRegImmOfs yet.

Add new hook TargetInstrInfo::getMemOpBaseRegImmOfs and implement for
x86. The implementation only handles a few easy cases now and will be
made more sophisticated in the future.

getMemOpBaseRegImmOfs, like getLdStBaseRegImmOfs, parses a memory
accessing instruction and tries to recover the memory being accessed as
a base pointer with a constant offset.

getMemOpBaseRegImmOfs is different from getLdStBaseRegImmOfs in the
sense that it also parses instructions that are not just loads and
stores, like "addl %ecx, (%rdi)" on x86.

[sanjoy]

ping

[ab]

include/llvm/Target/TargetInstrInfo.h

835–838 ↗

(On Diff #27008)

Seems to me this should just reuse getLdStBaseRegImmOfs. Judging by the user of that callback, the MachineScheduler, whether a load/store was folded or standalone isn't interesting.

Though if you override that for X86, it means the scheduler behavior might change, which is a whole 'nother can of worms.

Andy knows best; +Matthias, who I believe has been looking into the scheduler.

lib/Target/X86/X86InstrInfo.cpp

3967–3986 ↗

(On Diff #27008)

How about using X86II::getMemoryOperandNo() ?

isLoadingOp can be inferred from MC flags (MayLoad, or !MayStore, depending on what you're interested in).

[sanjoy]

include/llvm/Target/TargetInstrInfo.h

835–838 ↗

(On Diff #27008)

Though if you override that for X86, it means the scheduler behavior might change, which is a whole 'nother can of worms.

The only user of getLdStBaseRegImmOfs is LoadClusterMotion. LoadClusterMotion kicks in only if TargetInstrInfo::enableClusterLoads returns true, and it returns false for x86. So I think overriding getLdStBaseRegImmOfs for x86 should also be a NFC.

I did not override getLdStBaseRegImmOfs because the name seems to imply that we're parsing a load or a store; but I suppose just adding a clear comment should be fine here.

lib/Target/X86/X86InstrInfo.cpp

3967–3986 ↗

(On Diff #27008)

How about using X86II::getMemoryOperandNo() ?

Perfect, thanks for pointing that out!

[MatzeB]

Comment added below. Maybe you could comment on what you plan to use this for and hold off the commit until the using code is ready as well.

include/llvm/Target/TargetInstrInfo.h

835–838 ↗

(On Diff #27008)

I'd also recommend to not introduce a new API here, getLdStBaseRegImmOfs() is just too similar and as you already found, it won't change anything on X86.
It's of course sensible to rename the function. I would maybe not talk about folding in the comment but just about memory addressing modes (doesn't matter how they were produced).
Personally I would also prefer a "MachineInstr &" argument to indicate that nullptr is not a valid input but seeing that most other API here is using a pointer, I would be fine with the pointer version for consistency as well...

[atrick]

I agree. Convert getLdStBaseRegImmOfs into getMemOpBaseRegImmOfs. There is already a hook for shouldClusterLoads, so there's no reason to filter loads/stores in that API.

[sanjoy]

In D10199#185214, @MatzeB wrote:

Comment added below. Maybe you could comment on what you plan to use this for and hold off the commit until the using code is ready as well.

The motivation for this is http://reviews.llvm.org/D10201. I don't plan to check this in now, it will go in together with D10201.

[sanjoy]

Address review:

• unify with getLdStBaseRegImmOfs
• use getMemoryOperandNo

[MatzeB]

LGTM

[atrick]

LGTM too.

llvm/trunk/include/llvm/Target/TargetInstrInfo.h

public:
   /// to schedule the two loads together. "NumLoads" is the number of loads that
   /// have already been scheduled after Load1.
   virtual bool shouldScheduleLoadsNear(SDNode *Load1, SDNode *Load2,
                                        int64_t Offset1, int64_t Offset2,
                                        unsigned NumLoads) const {
     return false;
   }
 
-  /// Get the base register and byte offset of a load/store instr.
-  virtual bool getLdStBaseRegImmOfs(MachineInstr *LdSt,
-                                    unsigned &BaseReg, unsigned &Offset,
+  /// Get the base register and byte offset of an instruction that reads/writes
+  /// memory.
+  virtual bool getMemOpBaseRegImmOfs(MachineInstr *MemOp, unsigned &BaseReg,
+                                     unsigned &Offset,
                                      const TargetRegisterInfo *TRI) const {
     return false;
   }
 
   virtual bool enableClusterLoads() const { return false; }
 
   virtual bool shouldClusterLoads(MachineInstr *FirstLdSt,
                                   MachineInstr *SecondLdSt,
                                   unsigned NumLoads) const {

llvm/trunk/lib/CodeGen/MachineScheduler.cpp

 
 void LoadClusterMutation::clusterNeighboringLoads(ArrayRef<SUnit*> Loads,
                                                   ScheduleDAGMI *DAG) {
   SmallVector<LoadClusterMutation::LoadInfo,32> LoadRecords;
   for (unsigned Idx = 0, End = Loads.size(); Idx != End; ++Idx) {
     SUnit *SU = Loads[Idx];
     unsigned BaseReg;
     unsigned Offset;
-    if (TII->getLdStBaseRegImmOfs(SU->getInstr(), BaseReg, Offset, TRI))
+    if (TII->getMemOpBaseRegImmOfs(SU->getInstr(), BaseReg, Offset, TRI))
       LoadRecords.push_back(LoadInfo(SU, BaseReg, Offset));
   }
   if (LoadRecords.size() < 2)
     return;
   std::sort(LoadRecords.begin(), LoadRecords.end());
   unsigned ClusterLength = 1;
   for (unsigned Idx = 0, End = LoadRecords.size(); Idx < (End - 1); ++Idx) {
     if (LoadRecords[Idx].BaseReg != LoadRecords[Idx+1].BaseReg) {

llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.h

public:
 
   /// Return true if pairing the given load or store is hinted to be
   /// unprofitable.
   bool isLdStPairSuppressed(const MachineInstr *MI) const;
 
   /// Hint that pairing the given load or store is unprofitable.
   void suppressLdStPair(MachineInstr *MI) const;
 
-  bool getLdStBaseRegImmOfs(MachineInstr *LdSt, unsigned &BaseReg,
+  bool getMemOpBaseRegImmOfs(MachineInstr *LdSt, unsigned &BaseReg,
                              unsigned &Offset,
                              const TargetRegisterInfo *TRI) const override;
 
-  bool getLdStBaseRegImmOfsWidth(MachineInstr *LdSt, unsigned &BaseReg,
+  bool getMemOpBaseRegImmOfsWidth(MachineInstr *LdSt, unsigned &BaseReg,
                                   int &Offset, int &Width,
                                   const TargetRegisterInfo *TRI) const;
 
   bool enableClusterLoads() const override { return true; }
 
   bool shouldClusterLoads(MachineInstr *FirstLdSt, MachineInstr *SecondLdSt,
                           unsigned NumLoads) const override;
 
   bool shouldScheduleAdjacent(MachineInstr *First,
                               MachineInstr *Second) const override;

llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp

if (MIa->hasUnmodeledSideEffects() || MIb->hasUnmodeledSideEffects() ||
       MIa->hasOrderedMemoryRef() || MIb->hasOrderedMemoryRef())
     return false;
 
   // Retrieve the base register, offset from the base register and width. Width
   // is the size of memory that is being loaded/stored (e.g. 1, 2, 4, 8).  If
   // base registers are identical, and the offset of a lower memory access +
   // the width doesn't overlap the offset of a higher memory access,
   // then the memory accesses are different.
-  if (getLdStBaseRegImmOfsWidth(MIa, BaseRegA, OffsetA, WidthA, TRI) &&
-      getLdStBaseRegImmOfsWidth(MIb, BaseRegB, OffsetB, WidthB, TRI)) {
+  if (getMemOpBaseRegImmOfsWidth(MIa, BaseRegA, OffsetA, WidthA, TRI) &&
+      getMemOpBaseRegImmOfsWidth(MIb, BaseRegB, OffsetB, WidthB, TRI)) {
     if (BaseRegA == BaseRegB) {
       int LowOffset = OffsetA < OffsetB ? OffsetA : OffsetB;
       int HighOffset = OffsetA < OffsetB ? OffsetB : OffsetA;
       int LowWidth = (LowOffset == OffsetA) ? WidthA : WidthB;
       if (LowOffset + LowWidth <= HighOffset)
         return true;
     }
   }
void AArch64InstrInfo::suppressLdStPair(MachineInstr *MI) const {
 
   assert(MOSuppressPair < (1 << MachineMemOperand::MOTargetNumBits) &&
          "Too many target MO flags");
   (*MI->memoperands_begin())
       ->setFlags(MOSuppressPair << MachineMemOperand::MOTargetStartBit);
 }
 
 bool
-AArch64InstrInfo::getLdStBaseRegImmOfs(MachineInstr *LdSt, unsigned &BaseReg,
+AArch64InstrInfo::getMemOpBaseRegImmOfs(MachineInstr *LdSt, unsigned &BaseReg,
                                         unsigned &Offset,
                                         const TargetRegisterInfo *TRI) const {
   switch (LdSt->getOpcode()) {
   default:
     return false;
   case AArch64::STRSui:
   case AArch64::STRDui:
   case AArch64::STRQui:
   case AArch64::STRXui:
   case AArch64::STRWui:
   case AArch64::LDRSui:
   case AArch64::LDRDui:
   case AArch64::LDRQui:
   case AArch64::LDRXui:
   case AArch64::LDRWui:
     if (!LdSt->getOperand(1).isReg() || !LdSt->getOperand(2).isImm())
       return false;
     BaseReg = LdSt->getOperand(1).getReg();
     MachineFunction &MF = *LdSt->getParent()->getParent();
     unsigned Width = getRegClass(LdSt->getDesc(), 0, TRI, MF)->getSize();
     Offset = LdSt->getOperand(2).getImm() * Width;
     return true;
   };
 }
 
-bool AArch64InstrInfo::getLdStBaseRegImmOfsWidth(
+bool AArch64InstrInfo::getMemOpBaseRegImmOfsWidth(
     MachineInstr *LdSt, unsigned &BaseReg, int &Offset, int &Width,
     const TargetRegisterInfo *TRI) const {
   // Handle only loads/stores with base register followed by immediate offset.
   if (LdSt->getNumOperands() != 3)
     return false;
   if (!LdSt->getOperand(1).isReg() || !LdSt->getOperand(2).isImm())
     return false;
 
bool AArch64InstrInfo::getMemOpBaseRegImmOfsWidth(
 
   BaseReg = LdSt->getOperand(1).getReg();
   Offset = LdSt->getOperand(2).getImm() * Scale;
   return true;
 }
 
 /// Detect opportunities for ldp/stp formation.
 ///
-/// Only called for LdSt for which getLdStBaseRegImmOfs returns true.
+/// Only called for LdSt for which getMemOpBaseRegImmOfs returns true.
 bool AArch64InstrInfo::shouldClusterLoads(MachineInstr *FirstLdSt,
                                           MachineInstr *SecondLdSt,
                                           unsigned NumLoads) const {
   // Only cluster up to a single pair.
   if (NumLoads > 1)
     return false;
   if (FirstLdSt->getOpcode() != SecondLdSt->getOpcode())
     return false;
-  // getLdStBaseRegImmOfs guarantees that oper 2 isImm.
+  // getMemOpBaseRegImmOfs guarantees that oper 2 isImm.
   unsigned Ofs1 = FirstLdSt->getOperand(2).getImm();
   // Allow 6 bits of positive range.
   if (Ofs1 > 64)
     return false;
   // The caller should already have ordered First/SecondLdSt by offset.
   unsigned Ofs2 = SecondLdSt->getOperand(2).getImm();
   return Ofs1 + 1 == Ofs2;
 }

llvm/trunk/lib/Target/AArch64/AArch64StorePairSuppress.cpp

bool AArch64StorePairSuppress::runOnMachineFunction(MachineFunction &MF) {
   for (auto &MBB : MF) {
     bool SuppressSTP = false;
     unsigned PrevBaseReg = 0;
     for (auto &MI : MBB) {
       if (!isNarrowFPStore(MI))
         continue;
       unsigned BaseReg;
       unsigned Offset;
-      if (TII->getLdStBaseRegImmOfs(&MI, BaseReg, Offset, TRI)) {
+      if (TII->getMemOpBaseRegImmOfs(&MI, BaseReg, Offset, TRI)) {
         if (PrevBaseReg == BaseReg) {
           // If this block can take STPs, skip ahead to the next block.
           if (!SuppressSTP && shouldAddSTPToBlock(MI.getParent()))
             break;
           // Otherwise, continue unpairing the stores in this block.
           DEBUG(dbgs() << "Unpairing store " << MI << "\n");
           SuppressSTP = true;
           TII->suppressLdStPair(&MI);

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h

public:
 
   bool isReallyTriviallyReMaterializable(const MachineInstr *MI,
                                          AliasAnalysis *AA) const override;
 
   bool areLoadsFromSameBasePtr(SDNode *Load1, SDNode *Load2,
                                int64_t &Offset1,
                                int64_t &Offset2) const override;
 
-  bool getLdStBaseRegImmOfs(MachineInstr *LdSt,
-                            unsigned &BaseReg, unsigned &Offset,
+  bool getMemOpBaseRegImmOfs(MachineInstr *LdSt, unsigned &BaseReg,
+                             unsigned &Offset,
                              const TargetRegisterInfo *TRI) const final;
 
   bool shouldClusterLoads(MachineInstr *FirstLdSt,
                           MachineInstr *SecondLdSt,
                           unsigned NumLoads) const final;
 
   void copyPhysReg(MachineBasicBlock &MBB,
                    MachineBasicBlock::iterator MI, DebugLoc DL,
                    unsigned DestReg, unsigned SrcReg,

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp

static bool isStride64(unsigned Opc) {
   case AMDGPU::DS_WRITE2ST64_B32:
   case AMDGPU::DS_WRITE2ST64_B64:
     return true;
   default:
     return false;
   }
 }
 
-bool SIInstrInfo::getLdStBaseRegImmOfs(MachineInstr *LdSt,
-                                       unsigned &BaseReg, unsigned &Offset,
+bool SIInstrInfo::getMemOpBaseRegImmOfs(MachineInstr *LdSt, unsigned &BaseReg,
+                                        unsigned &Offset,
                                         const TargetRegisterInfo *TRI) const {
   unsigned Opc = LdSt->getOpcode();
   if (isDS(Opc)) {
     const MachineOperand *OffsetImm = getNamedOperand(*LdSt,
                                                       AMDGPU::OpName::offset);
     if (OffsetImm) {
       // Normal, single offset LDS instruction.
       const MachineOperand *AddrReg = getNamedOperand(*LdSt,
                                                       AMDGPU::OpName::addr);
static bool offsetsDoNotOverlap(int WidthA, int OffsetA,
   return LowOffset + LowWidth <= HighOffset;
 }
 
 bool SIInstrInfo::checkInstOffsetsDoNotOverlap(MachineInstr *MIa,
                                                MachineInstr *MIb) const {
   unsigned BaseReg0, Offset0;
   unsigned BaseReg1, Offset1;
 
-  if (getLdStBaseRegImmOfs(MIa, BaseReg0, Offset0, &RI) &&
-      getLdStBaseRegImmOfs(MIb, BaseReg1, Offset1, &RI)) {
+  if (getMemOpBaseRegImmOfs(MIa, BaseReg0, Offset0, &RI) &&
+      getMemOpBaseRegImmOfs(MIb, BaseReg1, Offset1, &RI)) {
     assert(MIa->hasOneMemOperand() && MIb->hasOneMemOperand() &&
            "read2 / write2 not expected here yet");
     unsigned Width0 = (*MIa->memoperands_begin())->getSize();
     unsigned Width1 = (*MIb->memoperands_begin())->getSize();
     if (BaseReg0 == BaseReg1 &&
         offsetsDoNotOverlap(Width0, Offset0, Width1, Offset1)) {
       return true;
     }

llvm/trunk/lib/Target/X86/X86InstrInfo.h

bool findCommutedOpIndices(MachineInstr *MI, unsigned &SrcOpIdx1,
                              unsigned &SrcOpIdx2) const override;
 
   // Branch analysis.
   bool isUnpredicatedTerminator(const MachineInstr* MI) const override;
   bool AnalyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
                      MachineBasicBlock *&FBB,
                      SmallVectorImpl<MachineOperand> &Cond,
                      bool AllowModify) const override;
+
+  bool getMemOpBaseRegImmOfs(MachineInstr *LdSt, unsigned &BaseReg,
+                             unsigned &Offset,
+                             const TargetRegisterInfo *TRI) const override;
   unsigned RemoveBranch(MachineBasicBlock &MBB) const override;
   unsigned InsertBranch(MachineBasicBlock &MBB, MachineBasicBlock *TBB,
                         MachineBasicBlock *FBB, ArrayRef<MachineOperand> Cond,
                         DebugLoc DL) const override;
   bool canInsertSelect(const MachineBasicBlock&, ArrayRef<MachineOperand> Cond,
                        unsigned, unsigned, int&, int&, int&) const override;
   void insertSelect(MachineBasicBlock &MBB,
                     MachineBasicBlock::iterator MI, DebugLoc DL,

llvm/trunk/lib/Target/X86/X86InstrInfo.cpp

case 64:
     assert(X86::VR512RegClass.hasSubClassEq(RC) && "Unknown 64-byte regclass");
     if (isStackAligned)
       return load ? X86::VMOVAPSZrm : X86::VMOVAPSZmr;
     else
       return load ? X86::VMOVUPSZrm : X86::VMOVUPSZmr;
   }
 }
 
+bool X86InstrInfo::getMemOpBaseRegImmOfs(MachineInstr *MemOp, unsigned &BaseReg,
+                                         unsigned &Offset,
+                                         const TargetRegisterInfo *TRI) const {
+  const MCInstrDesc &Desc = MemOp->getDesc();
+  int MemRefBegin = X86II::getMemoryOperandNo(Desc.TSFlags, MemOp->getOpcode());
+  if (MemRefBegin < 0)
+    return false;
+
+  MemRefBegin += X86II::getOperandBias(Desc);
+
+  BaseReg = MemOp->getOperand(MemRefBegin + X86::AddrBaseReg).getReg();
+  if (MemOp->getOperand(MemRefBegin + X86::AddrScaleAmt).getImm() != 1)
+    return false;
+
+  if (MemOp->getOperand(MemRefBegin + X86::AddrIndexReg).getReg() !=
+      X86::NoRegister)
+    return false;
+
+  const MachineOperand &DispMO = MemOp->getOperand(MemRefBegin + X86::AddrDisp);
+
+  // Displacement can be symbolic
+  if (!DispMO.isImm())
+    return false;
+
+  Offset = DispMO.getImm();
+
+  return (MemOp->getOperand(MemRefBegin + X86::AddrIndexReg).getReg() ==
+          X86::NoRegister);
+}
+
 static unsigned getStoreRegOpcode(unsigned SrcReg,
                                   const TargetRegisterClass *RC,
                                   bool isStackAligned,
                                   const X86Subtarget &STI) {
   return getLoadStoreRegOpcode(SrcReg, RC, isStackAligned, STI, false);
 }