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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg accepted this revision.
thegameg added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
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:
> > > 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 ?
> Right, it makes sense not to add it to `SavedRegs`. For compact unwinding we prefer storing registers in pairs to avoid encoding every single register in the unwind info, but in this case we definitely don't have support for `mostcc`. The supported registers are: x19 to x28 and d8 to d15, so in this case we will fallback on dwarf. I think this should be enough to fix it:
> 
> ```
>      // MachO's compact unwind format relies on all registers being stored in
>      // pairs.
>      // FIXME: the usual format is actually better if unwinding isn't needed.
> -    if (produceCompactUnwindFrame(MF) && !SavedRegs.test(PairedReg)) {
> +    if (PairedReg != AArch64::NoRegister && produceCompactUnwindFrame(MF) &&
> +        !SavedRegs.test(PairedReg)) {
>        SavedRegs.set(PairedReg);
>        if (AArch64::GPR64RegClass.contains(PairedReg) &&
>            !RegInfo->isReservedReg(MF, PairedReg))
> ```
You were faster than me commenting on this :) Thanks!


https://reviews.llvm.org/D51479