[PATCH] D12681: Calling conventions for HHVM

[Maksim Panchenko via llvm-commits]

maksfb added a comment.

Thanks for the review. The stack skew is due to the deviation from x64 ABI and is related to the way we enter our translation cache which happens from a piece of assembly code. Typically on a function entry `%rsp mod 16 == 8`, for us it's `%rsp mod 16 == 0`. From LLVM point of view all the objects on the stack with alignment over 8 bytes have to be placed differently relative to %rsp. Also, if there happens to be a call to a regular function, we don't have to adjust `%rsp` when the stack frame is empty (on x64 you have to issue a dummy `push` or `sub 8, %rsp`). There's more info in inline comments on why we have to change the alignment rules for implementation.


================
Comment at: include/llvm/IR/CallingConv.h:154
@@ +153,3 @@
+    /// perform calls to and from translation cache, and for calling PHP
+    /// functions. It can use most of GP registers for arguments and
+    /// return values.
----------------
reames wrote:
> This sentence isn't clear to me.  Is this analogous to anyreg where the register allocator can put arguments in any register and the runtime has to parse the stack map to figure out where they are?  Or are there defined argument and return registers?  If the later, I'd just emit this sentence.  
Unlike anyreg where LLVM chooses the registers, we'd like to give a similar level of control to the runtime (i.e. HHVM). If you think this sentence is confusing, I will remove it.

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp:50
@@ -49,3 +49,3 @@
 
-/// EmitULEB128 - emit the specified signed leb128 value.
+/// EmitULEB128 - emit the specified unsigned leb128 value.
 void AsmPrinter::EmitULEB128(uint64_t Value, const char *Desc,
----------------
reames wrote:
> Is this a stray change?  Or an unrelated bug fix?  If the former, please remove.  If the later, please submit separately.  
Documentation bugfix.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:515
@@ -514,3 +514,3 @@
   // Adjust to alignment boundary.
-  Offset = (Offset + Align - 1) / Align * Align;
+  Offset = RoundUpToAlignment(Offset, Align, Skew);
 
----------------
reames wrote:
> It would be cleaner to separate a refactoring change which used the helper function, then extended the argument list later.  I'd prefer you did this.
Sounds good. I will submit refactoring change separately.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:566
@@ -565,1 +565,3 @@
 
+  // For HHVM calling convention we need to apply skew to alignment of all
+  // objects on the stack because enter the translation cache at a different
----------------
reames wrote:
> I don't have a concrete objection here, but this section feels suspicious.  Why do you need to skew each and every element within the frame layout rather than inserting a dummy slot to ensure the skew is respected?  Also, do you really want individual arguments passed at Align + Skew?  Having an vector argument being passed at align (32) + 8 just seems weird to me.  
That's exactly what we need to do. Inserting dummy object wouldn't give us the same result. On x64 you need to push 8 bytes on a stack before making a call. Our stack have already been adjusted on the entry to HHVM translation cache function, so we can simply do the call without doing the stack adjustment. This changes alignment rules for all the objects on the stack though.


http://reviews.llvm.org/D12681