[PATCH] D12681: Calling conventions for HHVM

[Philip Reames via llvm-commits]

reames added a comment.

Most of this looks pretty straight forward.  The only part which worries me is the Skew implementation and how much that has to touch.  Could you give a bit of background on why you need this skew, and why you chose to implement it this way?

Fair warning, I'm not going to be able to sign off on the skew parts.  Everything else is in areas I'm familiar with, but that's really not.


================
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.
----------------
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.  

================
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,
----------------
Is this a stray change?  Or an unrelated bug fix?  If the former, please remove.  If the later, please submit separately.  

================
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);
 
----------------
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.

================
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
----------------
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.  


http://reviews.llvm.org/D12681