[PATCH] D51479: [AArch64] Implement aarch64_vector_pcs codegen support.

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added a comment.

Thanks for working on this! Other than the comments I left it looks good to me.



================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:539
   // All generated opcodes have scaled offsets.
-  assert(LocalStackSize % 8 == 0);
+  assert(LocalStackSize % 16 == 0);
   OffsetOpnd.setImm(OffsetOpnd.getImm() + LocalStackSize / Scale);
----------------
Should this be `% Scale`?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1524
+  auto getCSStackSize = [&CSRegs, &SavedRegs]() {
+    unsigned Size = 0;
+    for (unsigned i = 0; CSRegs[i]; ++i)
----------------
Would this also work?

```
for (unsigned Reg : SavedRegs.set_bits())
  Size += TRI.getRegSizeInBits(Reg, MRI);
```

or something like `std::accumulate`.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1592
+  // Recalculate the size of the CSRs
+  unsigned CSStackSize = getCSStackSize();
+  unsigned AlignedCSStackSize = alignTo(CSStackSize, 16);
----------------
Why do we need to recalculate this here? Can't we just update it along the way?


================
Comment at: test/CodeGen/AArch64/aarch64-vector-pcs.ll:29
+  call void asm sideeffect "nop", "~{x19}"()
+  call void asm sideeffect "nop", "~{q10},~{q11}"()
+  ret void
----------------
I think one nice way of testing this is using MIR.

You can write things like:

```
$x19 = IMPLICIT_DEF
$q10 = IMPLICIT_DEF
$q11 = IMPLICIT_DEF
```

and use `llc -run-pass prologepilog` or `llc -start-before prologepilog` to run it.

You can also look at `test/CodeGen/AArch64/reverse-csr-restore-seq.mir`. You might also want to check the `stack:` entries for the size and alignment like it's done in `test/CodeGen/AArch64/spill-stack-realignment.mir`.


https://reviews.llvm.org/D51479