[PATCH] D72758: Add OffsetIsScalable to getMemOperandWithOffset

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1245
                                        int64_t &Offset,
+                                       bool &OffsetIsScalable,
                                        const TargetRegisterInfo *TRI) const {
----------------
sdesmalen wrote:
> 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)`).
Oh, that makes sense.

I guess there are theoretically two separate dimensions: whether the width is scalable, and whether the offset is scalable.  For SVE, I guess those happen to be the same for all the defined instructions?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:957
+    if (BaseOpA->isIdenticalTo(*BaseOpB) &&
+        OffsetAIsScalable == OffsetBIsScalable) {
       int LowOffset = OffsetA < OffsetB ? OffsetA : OffsetB;
----------------
sdesmalen wrote:
> 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.
Oh, I didn't understand the part about the width; maybe worth adding a comment to the declaration of  getMemOperandWithOffsetWidth.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72758