This patch adds codegen support for the saving/restoring
V8-V23 for functions specified with the aarch64_vector_pcs
calling convention attribute, as added in patch D51477.
Details
Diff Detail
Event Timeline
Thanks for working on this! Other than the comments I left it looks good to me.
lib/Target/AArch64/AArch64FrameLowering.cpp | ||
---|---|---|
538 | Should this be % Scale? | |
1523 | Would this also work? for (unsigned Reg : SavedRegs.set_bits()) Size += TRI.getRegSizeInBits(Reg, MRI); or something like std::accumulate. | |
1588 | Why do we need to recalculate this here? Can't we just update it along the way? | |
test/CodeGen/AArch64/aarch64-vector-pcs.ll | ||
29 ↗ | (On Diff #163283) | 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. |
lib/Target/AArch64/AArch64FrameLowering.cpp | ||
---|---|---|
538 | I think it should, well spotted! | |
1523 | 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). | |
1588 | No need to recalculate indeed, I've fixed it now. | |
test/CodeGen/AArch64/aarch64-vector-pcs.ll | ||
29 ↗ | (On Diff #163283) | 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. |
lib/Target/AArch64/AArch64FrameLowering.cpp | ||
---|---|---|
1523 | 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. | |
1588 | Could you add a comment explaining that the 8 here is not Scale because we only add GPRs? |
lib/Target/AArch64/AArch64FrameLowering.cpp | ||
---|---|---|
1523 | 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 ? | |
1588 | yes, will do! |
- Calculate CSStackSize by accumulating reg-size for each saved register.
- Only sets PairedReg in SavedRegs when it is not AArch64::NoRegister.
- Removed trailing whitespace from test.
lib/Target/AArch64/AArch64FrameLowering.cpp | ||
---|---|---|
1523 | 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)) |
LGTM, thanks!
lib/Target/AArch64/AArch64FrameLowering.cpp | ||
---|---|---|
1523 | You were faster than me commenting on this :) Thanks! |
Should this be % Scale?