[PATCH] D25022: Register Calling Convention - CodeGen Basic Additions

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.


> X86CallingConv.td:37
> +  let GPR_32 = [EAX, ECX, EDX, EDI, ESI];
> +  let GPR_64 = [RAX]; /* not actually used, but AssignToReg can't handle [] */
> +  let XMM = [XMM0, XMM1, XMM2, XMM3, XMM4, XMM5, XMM6, XMM7];

This is really odd. Maybe fix AssignToReg?

> X86CallingConv.td:53
> +def RC_X86_64_RegCall_Win : RC_X86_64_RegCall {
> +  let GPR_8 = [AL, CL, DL, DIL, SIL, R8B, R9B, R10B, R11B, R12B, R14B, R15B];
> +  let GPR_16 = [AX, CX, DX, DI, SI, R8W, R9W, R10W, R11W, R12W, R14W, R15W];

GRP_8 and GPR_16 are only used for return values, right?
When will you use anything but AL/AX to return?

> X86CallingConv.td:95
> +
> +    // TODO: Handle the case of long double (f80)
> +

If I'm reading this correctly, with this definition, you'll pass f80 in an incompatible way - which sounds fairly bad. The right thing to do would be to either implement it in one go, or leave a TODO, but refuse to compile cases which use f80.
Otherwise, once you want to fix this, you'll have an unsolvable compatibility problem, since you can't be compatible with both old versions of clang, and with ICC.

> X86CallingConv.td:125
> +    CCIfSubtarget<"is64Bit()", CCIfType<[f80], CCAssignToStack<0, 0>>>,
> +    CCIfType<[f80], CCAssignToStack<0, 4>>,
> +

Why <0, 0> vs. <0, 4>? Should the 4 have been 0 as well?

> X86CallingConv.td:128
> +    // MMX type gets 8 byte slot in stack , while alignment depends on target
> +    CCIfSubtarget<"is64Bit()", CCIfType<[x86mmx], CCAssignToStack<8, 8>>>,
> +    CCIfType<[x86mmx], CCAssignToStack<8, 4>>,

How can we even get here? Is regcall supported for FP on non-SSE platforms?

> X86CallingConv.td:141
> +    CCIfType<[v32i8, v16i16, v8i32, v4i64, v8f32, v4f64], 
> +      CCAssignToStack<32, 32 >>,
> +

Extra space.

> X86CallingConv.td:144
> +    // 512-bit vectors get 64-byte stack slots that are 64-byte aligned.
> +    CCIfType<[v16i32, v8i64, v16f32, v8f64], CCAssignToStack<64, 64 >>
> +]>;

Extra space.

> X86CallingConv.td:156
> +
> +    // TODO: Handle the case of mask types (v*i1)
> +    // TODO: Handle the case of 32 bit machine with v64i1 argument 

Same as for the fp80 case - leaving TODOs in calling conventions is not generally a good idea, unless the temporary behavior is "fail".

> avx512-regcall-NoMask.ll:347
> +; Test regcall when passing/retrieving 256 bit vector
> +define x86_regcallcc <8 x i32> @test_CallargRet256Vector(<8 x i32> %a)  {
> +  %b = call x86_regcallcc <8 x i32> @test_argRet256Vector(<8 x i32> %a, <8 x i32> %a)

Have you checked what happens when you try to pass a 256-bit vector without AVX, 512-bit without AVX512, etc?
I assume there's no compatibility requirement for these cases, but it'd be good to check it does something that makes sense.

> sse-regcall.ll:208
> +}
> \ No newline at end of file


Please add a newline.

Repository:
  rL LLVM

https://reviews.llvm.org/D25022