[PATCH] D119338: [AArch64][SVE] Add structured load/store opcodes to getMemOpInfo

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen requested changes to this revision.
sdesmalen added a comment.
This revision now requires changes to proceed.

Hi @kmclaughlin, thanks for updating the tests as MIR tests, these look really good now! I've still requested changes on the patch, because the scaling doesn't look right yet. It seems like this offsets the base pointer with the wrong value/offset. It should be pretty trivial to fix that though.



================
Comment at: llvm/test/CodeGen/AArch64/sve-ldN.mir:94-95
+    ; CHECK-NEXT: renamable $z0_z1 = LD2B_IMM renamable $p0, killed $x8, -16
+    ; CHECK-NEXT: $x8 = ADDVL_XXI $sp, 4
+    ; CHECK-NEXT: renamable $z0_z1 = LD2B_IMM renamable $p0, killed $x8, 14
+    ; CHECK-NEXT: $x8 = ADDVL_XXI $sp, -4
----------------
I don't think this is correct, as I expect this to be a total offset of ptr + 2 + 14.

i.e. an ld2w instruction loads 2 vectors worth of data. The immediate is a multiple of 2, so the maximum immediate value to load from is ptr + 14 * sizeof(vector). When we load 2 * sizeof(vector) beyond that, I would expect it to load from ptr + 16, not ptr + 18. It depends on where the scaling is done. In this case, I think the scaling for this operand should be `TypeSize::Scalable(16)`, because the immediate is already expected to be scaled (it is already a multiple of 2/3/4). The encoding of the immediate will then divide the offset by 2, 3 or 4.

If we would have implemented the operand as an operand that is not yet scaled, then we'd need to do the scaling here, as well as in the operand printer.


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

https://reviews.llvm.org/D119338