[PATCH] D71655: [MachineScheduler] Allow clustering mem ops with complex addresses

[Jay Foad via Phabricator via llvm-commits]

foad marked an inline comment as done.
foad added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1262
+  /// passing into shouldClusterMemOps. On failure, return no operands at all.
+  virtual void getMemAddressOperands(const MachineInstr &MI,
+                                     SmallVectorImpl<const MachineOperand *> &AddrOps,
----------------
arsenm wrote:
> Why doesn't this one return bool for failure?
Returning without adding anything to `AddrOps` signifies failure. I'm happy to change it to return a bool if you think it'd be clearer.

I've been pondering other changes to this API like:
* Returning the offset //in bytes// as an int64_t, like the getMemBase* functions do. Then `AddrOps` would only be for base (register/FI) operands.
* Also return the width in bytes of the memory access. Several targets already have internal functions that do this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71655/new/

https://reviews.llvm.org/D71655