[PATCH] [TargetInstrInfo] Add new hook: getMemOpBaseRegImmOfs.

[Matthias Braun]

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.


================
Comment at: include/llvm/Target/TargetInstrInfo.h:835-838
@@ -834,1 +834,6 @@
 
+  /// 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,
----------------
sanjoy wrote:
> ab wrote:
> > 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.
> > 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.
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...

http://reviews.llvm.org/D10199

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/