On AArch64, s128 types have to be split into s64 GPRs when passed as arguments. This change adds the generic support in call lowering for dealing with multiple registers, for incoming and outgoing args. Support for splitting for return types not yet implemented.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I'm somewhat confused by what the responsibilities of handleAssignments are vs. target code. I already have multiple register breakdowns without this patch in AMDGPU
IIUC calling convention specified register splits should be generic, and it's handled in generic code by SelectionDAG. Whether handleAssignments is the best place for I'm not sure, but I don't think targets should have to deal with this individually. In arm64 I think we already have some target call lowering code that should really be generic.
llvm/lib/CodeGen/GlobalISel/CallLowering.cpp | ||
---|---|---|
198–200 ↗ | (On Diff #214953) | obligatory braces nit |
206 ↗ | (On Diff #214953) | I think a high-level explanation of what we're going to do when we have an incoming vs outgoing handler would be useful. |
220 ↗ | (On Diff #214953) | Can we have a comment here explaining what's going on? |
225–227 ↗ | (On Diff #214953) | I think that this could use a comment too. |
232 ↗ | (On Diff #214953) | Why less than 2? |
232–235 ↗ | (On Diff #214953) | braces |
285 ↗ | (On Diff #214953) | Pull Args[I].Regs.size() into a variable with a meaningful name? |
llvm/test/CodeGen/AArch64/GlobalISel/arm64-callingconv.ll | ||
1 ↗ | (On Diff #214953) | Why are we falling back now? |
llvm/lib/CodeGen/GlobalISel/CallLowering.cpp | ||
---|---|---|
232 ↗ | (On Diff #214953) | Because this is a code path that hasn't been implemented or tested yet. That is, if we're dealing with an outgoing arg, and we failed to assign a location (at the beginning of handleAssignments), and the calling convention says it doesn't need to be split (NumParts < 2), then we don't know what else to do. |
llvm/test/CodeGen/AArch64/GlobalISel/arm64-callingconv.ll | ||
1 ↗ | (On Diff #214953) | We're not, I'm just adding this flag to ensure we error if we do fall back at all. |
llvm/include/llvm/CodeGen/GlobalISel/CallLowering.h | ||
---|---|---|
196 ↗ | (On Diff #214953) | It looks like you only use the ability here to change elements, not add/remove them? I think that's a MutableArrayRef use-case. |
llvm/lib/CodeGen/GlobalISel/CallLowering.cpp | ||
165 ↗ | (On Diff #216514) | It looks like you only modify elements in place? If so, that's MutableArrayRef's thing. |
260 ↗ | (On Diff #216514) | Why only setSplit for outgoing args? |
301 ↗ | (On Diff #216514) | Assertion? |
llvm/lib/CodeGen/GlobalISel/CallLowering.cpp | ||
---|---|---|
260 ↗ | (On Diff #216514) | No good reason, I'll add it. |
llvm/include/llvm/CodeGen/GlobalISel/CallLowering.h | ||
---|---|---|
196 ↗ | (On Diff #214953) | Right, but there's no conversion for SmallVectors to MutableArrayRef, only SmallVectorImpl. |
llvm/trunk/lib/CodeGen/GlobalISel/CallLowering.cpp | ||
---|---|---|
289 | After this commit, I'm seeing this assert firing. test.c: Compiled with: |
After this commit, I'm seeing this assert firing.
test.c:
typedef struct { int128_t m3 } struct2;
typedef struct { uint128_t m2 } union12;
fn1(int p1, struct2 p2, p3, p4, p5, union12 p6, p7) {}
Compiled with:
clang -cc1 -triple aarch64-arm-none-eabi -emit-obj -O0 -o test.o test.c