[PATCH] D72758: Add OffsetIsScalable to getMemOperandWithOffset

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 3 inline comments as done.
sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1245
                                        int64_t &Offset,
+                                       bool &OffsetIsScalable,
                                        const TargetRegisterInfo *TRI) const {
----------------
efriedma wrote:
> I'm not sure why this modification is necessary.  The offset doesn't actually scale, does it?  The width of memory operation does, but getMemOperandWithOffset doesn't return that anyway.
For e.g. `AArch64::LDR_ZXI` and `AArch64::STR_ZXI`, it is actually the offset that is scaled by VL elements (i.e. load/store at `ptr + (N * sizeof(vector)`).


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:957
+    if (BaseOpA->isIdenticalTo(*BaseOpB) &&
+        OffsetAIsScalable == OffsetBIsScalable) {
       int LowOffset = OffsetA < OffsetB ? OffsetA : OffsetB;
----------------
efriedma wrote:
> Is computing overlap on scalable vectors here really sound?
I think so; it assumes that the runtime scaling of OffsetA is the same as that of OffsetB. With runtime scaling being `vscale` for both vectors and the `width` being the worst-case width (256 bytes), I think we can determine whether there is no overlap. The function seems otherwise conservative and assumes overlap.


================
Comment at: llvm/lib/Target/Lanai/LanaiInstrInfo.h:78
                                     int64_t &Offset, unsigned &Width,
+                                    bool &OffsetIsScalable,
                                     const TargetRegisterInfo *TRI) const;
----------------
efriedma wrote:
> Do we need to change getMemOperandWithOffsetWidth now?  It's a target-specific method.
Good point, that indeed seems unnecessary. I'll remove those changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72758