ARM, PR15868 Release-blocker fix. ByVal parameters padding fix.

Manman Ren mren at apple.com
Mon May 13 16:24:01 PDT 2013


There seems to be an issue when we have a small byval of 4-byte followed by a large byval of 200-byte.
The patch will not use padding area for the first small byval, and will use 4-byte padding for the second byval.
This will cause the callee to update SP by 20 bytes (ArgRegsSaveSize will be 4 + 4 + 4*3), which means SP will not be 8-byte aligned.

+    // If parameter was splitted between stack and GPRs...
+    if ((ArgRegsSize < ArgSize ||
+         InRegsParamRecordIdx >= CCInfo.getInRegsParamsCount()) &&
+        ArgRegsSize != ArgRegsSaveSize) {
+      // Add padding for part of param recovered from GPRs, so
+      // its last byte must be at address K*8 - 1.
+      // We need to do it, since remained (stack) part of parameter has
+      // stack alignment, and we need to "attach" "GPRs head" without gaps
+      // to it:
+      // Stack:
+      // |---- 8 bytes block ----| |---- 8 bytes block ----| |---- 8 bytes...
+      // [ [padding] [GPRs head] ] [        Tail passed via stack       ....
+      //
+      Padding = ArgRegsSaveSize - ArgRegsSize;
+
+      // We save this padding, since we need to correct *all* ArgOffsets
+      // that are going after this one.
+      AFI->setStoredByValParamsPadding(Padding);
+    } else
+      ArgRegsSaveSize = ArgRegsSize;

I would prefer to update ArgRegsSaveSize correctly inside computeRegArea, instead of updating it again here in the else clause.

@@ -2756,7 +2782,7 @@
   // argument passed via stack.
   int FrameIndex =
     StoreByValRegs(CCInfo, DAG, dl, Chain, 0, CCInfo.getInRegsParamsCount(),
-                   0, ArgOffset, ForceMutable);
+                   0, ArgOffset, 4, ForceMutable);

It is a little weird to have a hard-coded ArgSize 4 here for a variable argument.

Thanks for working on this,
Manman

On May 13, 2013, at 6:26 AM, Stepan Dyatkovskiy wrote:

> Hi all,
> 
> In case when stack alignment is 8 and GPRs parameter part size is not N*8: we add padding to GPRs part, so part's last byte must be recovered at address K*8-1.
> We need to do it, since remained (stack) part of parameter starts from address K*8, and we need to "attach" "GPRs head" without gaps to it:
> Stack:
> |---- 8 bytes block ----| |---- 8 bytes block ----| |---- 8 bytes...
> [ [padding] [GPRs head] ] [ ------ Tail passed via stack  ------ ...
> 
> Note, once we added padding we need to correct *all* Arg offsets that are going after padded one.
> That's why we need this fix: Arg offsets were never corrected before this patch. See new test-case included in patch.
> 
> We also don't need to insert padding for byval parameters that are stored in GPRs only. We need pad only last byval parameter and only in case it outsides GPRs and stack alignment = 8.
> 
> Please find the patch in attachment.
> 
> This patch reduces stack usage for some cases:
> We can reduce ArgRegsSaveArea since inner 4 bytes sized byval params my be "packed" with alignment 4.
> 
> This patch also fixes PR15868 release-blocking issue.
> 
> -Stepan.
> 
> <pr15868-2013-05-13-2.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits




More information about the llvm-commits mailing list