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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1524
+  auto getCSStackSize = [&CSRegs, &SavedRegs]() {
+    unsigned Size = 0;
+    for (unsigned i = 0; CSRegs[i]; ++i)
----------------
thegameg wrote:
> sdesmalen wrote:
> > 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).
> I guess we could check for `!Reg` in the loop, or fix `getRegSizeInBits`. I am not sure if returning 0 in `getRegSizeInBits` would be a good idea but I will let others comment on that.
> Either way, it could be a good idea to add a comment + an assert in `getRegSizeInBits`.
Rather than fixing it up in this loop, I wonder if the condition 'PairedReg != AArch64::NoRegister' should be added to the condition on line 1513, so that it is never set as a SavedReg to begin with. (an assert is already present in getRegSizeInbits).

In the test that fails: `test/CodeGen/AArch64/preserve_mostcc.ll`, not all registers are stored in pairs (see `str x15`), which seems to contradict the reason for setting PairedReg according to the comment on line 1510. I don't really know enough about the MachO format to know whether this is a bug, so perhaps someone else can comment on that. @gberry ?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1588
+  // Recalculate the size of the CSRs
+  CSStackSize += 8 * (SavedRegs.count() - NumSavedRegs);
+  unsigned AlignedCSStackSize = alignTo(CSStackSize, 16);
----------------
thegameg wrote:
> Could you add a comment explaining that the `8` here is not `Scale` because we only add GPRs?
yes, will do!


https://reviews.llvm.org/D51479