[PATCH] D67749: [AArch64] Stackframe accesses to SVE objects.

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally accepted this revision.
cameron.mcinally marked an inline comment as done.
cameron.mcinally added a comment.
This revision is now accepted and ready to land.

LGTM with a moderate confidence level. Maybe give other reviewers a day or so to comment before committing.



================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3453
+    SOffset = StackOffset(Offset, MVT::i8) +
+              StackOffset(SOffset.getScalableBytes(), MVT::nxv1i8);
   return AArch64FrameOffsetCanUpdate |
----------------
sdesmalen wrote:
> cameron.mcinally wrote:
> > Would you shed some light on what this change is doing?
> > 
> > `IsMulVL` indicates there are scalable objects on the stack, right? What is the reason for the behavior change of the legacy code when `!IsMulVL`. I.e. the addition of `StackOffset(SOffset.getScalableBytes(), MVT::nxv1i8)` in the else block.
> `isAArch64FrameOffsetLegal` tries to determine if the immediate field of the instruction can fit the given StackOffset, and will attempt to fold as much of the offset in the immediate.
> 
> Although a StackOffset can contain both a scalable and a non-scalable part, it will depend on the instruction whether the immediate is scalable or non-scalable. For example, in
> ```LDR <Zt>, [<Xn|SP>{, #<imm>, MUL VL}]```
> the immediate is `mul vl`, so scalable, which means the instruction can only handle the "scalable" part of the StackOffset. The rest of the offset will need to be handled elsewhere.
> 
> The variable `int64_t Offset` uses either the scalable or non-scalable part of the StackOffset, which happens on line 3411. After that, this function does its magic to determine what part of the offset can be folded into the immediate.
> 
> On line 3448, the remaining part of the offset that could *not* be folded into the immediate will need to be reflected in the in/out parameter `SOffset`, which is a StackOffset.
> If `IsMulVL` is true, then variable `Offset` is scalable and will at this point contain the part of the scalable offset that could not be folded into the immediate.
> SOffset.getBytes() just passes through the fixed-size part of the offset that is not handled by the instruction.
> Conversely, if `IsMulVL` is false, then the variable `Offset` is non-scalable and will contain the part of the fixed-size offset that could not be folded into the immediate. It then has to pass through SOffset.getScalableBytes() that is not handled by the instruction, so it can be handled elsewhere.
> 
> For example:
> 
> ```isAArch64FrameOffsetLegal(AArch64::LDR_ZXI, {16, MVT::i8} + {16, MVT::nxv1i8})```
> would fold `{16, MVT::nxv1i8}` into the immediate, and the resulting SOffset would be `{16, MVT::i8}`. It would return `AArch64FrameOffsetCanUpdate`.
> 
> ```isAArch64FrameOffsetLegal(AArch64::LDR_ZXI, {16, MVT::i8} + {4096, MVT::nxv1i8})```
> would fold `{4080, MVT::nxv1i8}` into the immediate (note that it's immediate goes up to `#255 MUL VL`), and the resulting SOffset would be `{16, MVT::i8} + {16, MVT::nxv1i8}`.
Ah, okay. So it's determining the addressing mode...


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

https://reviews.llvm.org/D67749