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

[Sander de Smalen via Phabricator via llvm-commits]

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


================
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);
----------------
thegameg wrote:
> Should this be `% Scale`?
I think it should, well spotted!


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1524
+  auto getCSStackSize = [&CSRegs, &SavedRegs]() {
+    unsigned Size = 0;
+    for (unsigned i = 0; CSRegs[i]; ++i)
----------------
thegameg wrote:
> Would this also work?
> 
> ```
> for (unsigned Reg : SavedRegs.set_bits())
>   Size += TRI.getRegSizeInBits(Reg, MRI);
> ```
> 
> or something like `std::accumulate`.
I tried it and it seems to work, but one of the unit tests fails. I think this is because it exposes a bug where Reg is AArch64::NoRegister (set on line 1515 above to ensure registers are being stored in pairs for MachO's compact unwind format).


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


================
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
----------------
thegameg wrote:
> 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`.
Thanks for the suggestion. When I created the test initially I wasn't convinced that having a .mir test would add much benefit, but I think being able to check the alignment of the objects on the stack is a good reason to do so (rather than observing the effects in the resulting assembly). I've migrated the test to a .mir test in my latest revision.


https://reviews.llvm.org/D51479