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

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

It would be great if another person at Intel went through the test-cases, and verified that they really confirm to the ABI - I'm not familiar with the calling convention beyond what's documented, so...



================
Comment at: lib/Target/X86/X86CallingConv.h:51
+    " doesn't support long double and mask types yet.");
+  return false; // gracefully continue the search - fallback to stack.
+}
----------------
mkuper wrote:
> The whole point is that we don't gracefully continue. :-)
> Can you make this llvm_unreachable?
I'm sorry, what I meant was - keep report_fatal_error, but make the *return* llvm_unreachable, if possible (I don't remember if this elicits warnings or not.)


================
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];
----------------
oren_ben_simhon wrote:
> 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
Ok, I see. Both GCC and Clang accept this by default as an extension, but I guess MSVC doesn't (and maybe GCC/Clang don't either with the flags we use for LLVM).

Anyway, I'm ok with deferring fixing this to a separate patch. Please make the comment here into a TODO.


================
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>>,
----------------
oren_ben_simhon wrote:
> 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
Ok, thanks.


================
Comment at: lib/Target/X86/X86CallingConv.td:103
+    CCIfType<[v32i8, v16i16, v8i32, v4i64, v8f32, v4f64], 
+      CCIfSubtarget<"hasSSE1()", CCAssignToReg<RC.YMM>>>,
+
----------------
AVX? Or should this get passed as 2 * XMM if we have SSE but not AVX?


================
Comment at: lib/Target/X86/X86CallingConv.td:108
+    CCIfType<[v64i8, v32i16, v16i32, v8i64, v16f32, v8f64], 
+      CCIfSubtarget<"hasSSE1()",CCAssignToReg<RC.ZMM>>>,
+
----------------
AVX512?


================
Comment at: lib/Target/X86/X86CallingConv.td:170
+    CCIfType<[v32i8, v16i16, v8i32, v4i64, v8f32, v4f64], 
+      CCIfSubtarget<"hasSSE1()", CCAssignToReg<RC.YMM>>>,
+
----------------
Same as above.


================
Comment at: lib/Target/X86/X86CallingConv.td:174
+    CCIfType<[v64i8, v32i16, v16i32, v8i64, v16f32, v8f64], 
+      CCIfSubtarget<"hasSSE1()", CCAssignToReg<RC.ZMM>>>
+]>;
----------------
Same as above.


Repository:
  rL LLVM

https://reviews.llvm.org/D25022