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

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.


> X86CallingConv.h:51
> +    " doesn't support long double and mask types yet.");
> +  return false; // gracefully continue the search - fallback to stack.
> +}

The whole point is that we don't gracefully continue. :-)
Can you make this llvm_unreachable?

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

Can you remove this until you reintroduce f80 support?
I think leaving it in is just confusing at this point.

> oren_ben_simhon wrote in X86CallingConv.td:37
> AssignToReg, takes the list of registers and creates a static constant list of registers. For example:
> 
>   static const MCPhysReg RegList1[] = {
>     X86::ECX, X86::EDX, X86::R8D, X86::R9D
>   };
> 
> Empty static constant list is forbidden in C.
> 
> Changing the expansion of AssignToReg (to check if the list is empty) will require LLVM infrastructure change.
> I would prefer to leave it out of the scope of this review.

I agree this is not in scope of this review. The way we tend to do this is by first fixing the problem (in a separate review), and then using the fix in a the functional change.

In any case, I'm not sure I see what the problem is - an empty static constant array compiles just fine, e.g.
static const int foo[] = {};
What am I missing?

> oren_ben_simhon wrote in X86CallingConv.td:53
> You are right, GPR8/GPR16 are used just for return values.
> In regcall calling convention there are cases in which you return a value in multiple registers.
> Some examples:
> 
> - v64i1 (mask) in IA32 is splitted into two registers
> - a structure that is returned, will be splitted to multiple registers according to its fields.

Ah, ok, I see, thanks.

> oren_ben_simhon wrote in X86CallingConv.td:128
> Yes, regcall supports non-sse platforms. In that case, FP will be saved on the stack.

Is this behavior documented somewhere?
https://software.intel.com/en-us/node/693069 suggests float/double values are always classified as XMM.

> avx512-regcall-NoMask.ll:144
> +; X32-LABEL:  test_argReti64:
> +; X32:        addl $1, %eax
> +; X32:        ret{{.*}}

Perhaps a test that exercises this case more fully (modifies both registers)?

> avx512-regcall-NoMask.ll:180
> +; X32-LABEL:  test_argRetFloat:
> +; X32:        vadd{{.*}}  {{.*}}, {{%xmm([0-7])}}
> +; X32:        ret{{.*}}

In all the float/double/vector tests - shouldn't you be looking for a specific register in a lot of places?

> avx512-regcall-NoMask.ll:541
> +}
> +

I don't see any struct tests.
Do you expect the FE to completely break down structs for you?

If you do:

1. What about returning structs?
2. Would  this actually do the right thing for regcall? That is, can you pass "half a struct" in registers, and half on the stack? I assume you don't expect to use inreg.

> sse-regcall.ll:85
> +;test calling conventions - input parameters, callee saved XMMs
> +define x86_regcallcc <16 x float> @testf32_inp(<16 x float> %a, <16 x float> %b, <16 x float> %c) nounwind {
> +  %x1 = fadd <16 x float> %a, %b

This doesn't actually check how the "oversize" arguments are passed.
What I'm driving at here is that if this is supported (and documented) by ICC, we need to do it in a compatible way.

Repository:
  rL LLVM

https://reviews.llvm.org/D25022