[PATCH] D51108: [PowerPC] Fix wrong ABI for i1 stack arguments for PPC32

[Lion Yang via Phabricator via llvm-commits]

LionNatsu created this revision.
Herald added subscribers: llvm-commits, kbarton, nemanjai.

The current implementation with +crbits feature (enabled if >=https://reviews.llvm.org/owners/package/2/)
(caller) passes i1 stack arguments by writing a single byte on the offset
of the stack object and (callee) reads the single byte. Say we have two
boolean (i1) true in stack arguments:

  xx xx xx xx <- r1 (the pointer to the previous frame, big-endian)
  01 ?? ?? ??    +8 (stored 0x01 to r1+0x8)
  01 ?? ?? ??    +c (stored 0x01 to r1+0xC)

According to _PowerPC Processor ABI Supplement (September 1995)_, any type
smaller than i32 must be extended to i32 first. It had been implemented correctly
before adding +crbits feature:

  xx xx xx xx <- r1
  00 00 00 01    +8 (0x01 extended to 0x00000001 and stored in big-endian)
  00 00 00 01    +c (0x01 extended to 0x00000001 and stored in big-endian)

The +crbits (introduced at r202451) makes i1 a special case to handle,
which has only 8 bits to fill into the stack frame. It did not handle the
alignment properly. The result will be unknown if it loads/stores with different
sizes on the same address in a big-endian architecture.

This commit fixed the bug by:

1. Callee: adding offset (stack object size(4) - actual size(1) = 3 bytes) for MFI.CreateFixedObject() in LowerFormalArguments_32SVR4();
2. Caller: extending the i1 to i32 before passing, no matter VA.isRegLoc or VA.isMemLoc.


Repository:
  rL LLVM

https://reviews.llvm.org/D51108

Files:
  lib/Target/PowerPC/PPCISelLowering.cpp


Index: lib/Target/PowerPC/PPCISelLowering.cpp
===================================================================
--- lib/Target/PowerPC/PPCISelLowering.cpp
+++ lib/Target/PowerPC/PPCISelLowering.cpp
@@ -3505,9 +3505,14 @@
       // Argument stored in memory.
       assert(VA.isMemLoc());
 
+      // Get the extended size of the argument type in stack
       unsigned ArgSize = VA.getLocVT().getStoreSize();
-      int FI = MFI.CreateFixedObject(ArgSize, VA.getLocMemOffset(),
-                                     isImmutable);
+      // Get the actual size of the argument type
+      unsigned ObjSize = VA.getValVT().getStoreSize();
+      unsigned ArgOffset = VA.getLocMemOffset();
+      // Stack objects in PPC32 are right justified.
+      ArgOffset += ArgSize - ObjSize;
+      int FI = MFI.CreateFixedObject(ArgSize, ArgOffset, isImmutable);
 
       // Create load nodes to retrieve arguments from the stack.
       SDValue FIN = DAG.getFrameIndex(FI, PtrVT);
@@ -5462,10 +5467,11 @@
       Arg = PtrOff;
     }
 
-    if (VA.isRegLoc()) {
-      if (Arg.getValueType() == MVT::i1)
-        Arg = DAG.getNode(ISD::ZERO_EXTEND, dl, MVT::i32, Arg);
+    // Ensure callee will get either 0x00000001 or 0x00000000.
+    if (Arg.getValueType() == MVT::i1)
+      Arg = DAG.getNode(ISD::ZERO_EXTEND, dl, MVT::i32, Arg);
 
+    if (VA.isRegLoc()) {
       seenFloatArg |= VA.getLocVT().isFloatingPoint();
       // Put argument in a physical register.
       RegsToPass.push_back(std::make_pair(VA.getLocReg(), Arg));


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51108.161980.patch
Type: text/x-patch
Size: 1523 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180822/89349140/attachment.bin>