[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

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

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

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

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

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.

include/llvm/Target/TargetInstrInfo.h

public:
 
   /// Get the base register and byte offset of a load/store instr.
   virtual bool getLdStBaseRegImmOfs(MachineInstr *LdSt,
                                     unsigned &BaseReg, unsigned &Offset,
                                     const TargetRegisterInfo *TRI) const {
     return false;
   }
 
+  /// Get the base register and byte offset of an instruction that reads/writes
+  /// memory.  This is similar to getLdStBaseRegImmOfs, but also works on memory
+  /// instructions that have been folded into other non-memory operations, like
+  /// arithmetic.
+  virtual bool getMemOpBaseRegImmOfs(MachineInstr *MemOp, bool &IsLoadingOp,
+                                     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 {
     return false;
   }
 

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, bool &IsLoadOp,
+                             unsigned &BaseReg, unsigned &Offset,
+                             const TargetRegisterInfo *TRI) const override;
   unsigned RemoveBranch(MachineBasicBlock &MBB) const override;
   unsigned InsertBranch(MachineBasicBlock &MBB, MachineBasicBlock *TBB,
                         MachineBasicBlock *FBB,
                         const SmallVectorImpl<MachineOperand> &Cond,
                         DebugLoc DL) const override;
   bool canInsertSelect(const MachineBasicBlock&,
                        const SmallVectorImpl<MachineOperand> &Cond,
                        unsigned, unsigned, int&, int&, int&) const override;

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, bool &IsLoadingOp,
+                                         unsigned &BaseReg, unsigned &Offset,
+                                         const TargetRegisterInfo *TRI) const {
+  unsigned MemRefBegin = -1;
+
+  switch (MemOp->getOpcode()) {
+  default:
+    return false; // cannot parse this instruction
+
+  case X86::MOV64rm:
+  case X86::MOV32rm:
+  case X86::MOV16rm:
+    IsLoadingOp = true;
+    MemRefBegin = 1;
+    break;
+
+  case X86::ADD64rm:
+  case X86::ADD32rm:
+  case X86::ADD16rm:
+    IsLoadingOp = true;
+    MemRefBegin = 2;
+    break;
+  }
+
+  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);
 }