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

[Oren Ben Simhon via llvm-commits]

oren_ben_simhon marked 6 inline comments as done.
oren_ben_simhon added inline comments.


================
Comment at: lib/Target/X86/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];
----------------
mkuper wrote:
> oren_ben_simhon wrote:
> > mkuper wrote:
> > > This is really odd. Maybe fix AssignToReg?
> > 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?
I am not sure what is the difference between our environments (are you compiling for C instead for C++?), here is the error message that i get:
C:\work\obensimh\ICL\llorg_ws\builds\lib\Target\X86\X86GenCallingConv.inc(817): error C2466: cannot allocate an array of constant size 0

You can find more details in the following link:
http://stackoverflow.com/questions/9881777/why-do-i-get-cannot-allocate-an-array-of-constant-size-0


================
Comment at: lib/Target/X86/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>>,
----------------
mkuper wrote:
> oren_ben_simhon wrote:
> > mkuper wrote:
> > > How can we even get here? Is regcall supported for FP on non-SSE platforms?
> > 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.
I will make sure it is added to the documentation and add a comment in the code as well


================
Comment at: test/CodeGen/X86/avx512-regcall-NoMask.ll:541
+}
+
----------------
mkuper wrote:
> 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.
Yes, I do expect the structures to be broken down by FE.
However, the FE should break the structure down only if it can break down all of it. No partial break down allowed.
In other words, you can't pass half structure on stack and half on registers. In that case, the structure will be passed through a pointer.

You are right, we are not depending on inreg attribute.

In the case of returned structure, the behaviour is similar to passing a structure (that was broken down) to a function.
So actually, all tests that call a regcall function and pass to it parameters, have similar behaviour to tests that return a structure.
In order to make it clearer, I added a test that pass/return a structure by value.


================
Comment at: test/CodeGen/X86/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
----------------
mkuper wrote:
> 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.
If i undersand you correctly, test_f32stack checks for "oversize" arguments which are saved on the stack.
All tests are compatible with the ABI.


Repository:
  rL LLVM

https://reviews.llvm.org/D25022