[PowerPC] Fix wrong ABI for i1 stack arguments on PPC32

[LionNatsu]

Summary

The current implementation with +crbits feature (enabled if >=O2)
(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 patch fixes 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 in LowerCall_32SVR4(), no matter VA.isRegLoc or VA.isMemLoc.

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=38661

[nagisa]

Please add a test. See this.

Please ensure that patch contains full context. See this on how to generate a patch correctly.

[nagisa]

The minimal test case you added to the bug report should be fine.

[nagisa]

You also want to add some people as reviewers (possibly based on a blame of the file), otherwise the patch might go by unnoticed.

[LionNatsu]

Add reviewers, upload a test case and update the patch with full context.

[LionNatsu]

@nagisa Thank you for your help! :)

[hfinkel]

lib/Target/PowerPC/PPCISelLowering.cpp

5471 ↗

(On Diff #162003)

In your test case, the arguments have the zeroext attribute. That seems right. However, that being the case, should this be checking for i1, or should we check for Flags.isZExt()?

[LionNatsu]

lib/Target/PowerPC/PPCISelLowering.cpp

5471 ↗

(On Diff #162003)

Good suggestion. I am working on this now.
Making an i1 argument "sigext" means it will pass either 0xFFFFFFFF or 0x00000000.

But now the last question is which one should be the default behaviour, ZERO_EXTEND or SIGN_EXTEND?

PPC32 ABI said:

Values shorter than 32 bits are sign-extended or zero-extended, depending on whether they are signed or unsigned.

It is quite ambiguous for one-bit type because Spec did not mention how boolean should be implemented.

For comparison, PPC64 ELFv2 ABI also said:

Map simple integer types (char, short, int, long, enum) to a single doubleword. Sign or zero extend values shorter than a doubleword to a doubleword based on whether the source data type is signed or unsigned.

But additionally:

• A Boolean value is represented as a byte with a value of 0 or 1. If a byte with a value other than 0 or 1 is evaluated as a boolean value (for example, through the use of unions), the behavior is undefined.

So the byte of a *bool* should be either 0x01 or 0x00 hence signedness will be pointless on PPC64.

ISO C++ said:
In 6.7.1 Fundamental types

Values of type bool are either true or false. [ Note: There are no signed, unsigned, short, or long bool types or values. — end note ] Values of type bool participate in integral promotions.

In 7.3.6 Integral promotions

A prvalue of type bool can be converted to a prvalue of type int, with false becoming zero and true becoming one.

std::is_signed in C++ 11, returns T(-1) < T(0). -1 turned to true and true turned to 1 by integral promotions, 1 < 0 is false.

I think all these things suggest us to set zeroext as default. Any thought?

[LionNatsu]

Use SIGN_EXTEND in LowerCall_32SVR4 if explicitly specified, update summary.

[hfinkel]

lib/Target/PowerPC/PPCISelLowering.cpp

5471 ↗

(On Diff #162324)

But why is this check specific to i1? What about i8 or i16? I'm assuming that this is because i1 is the only type smaller than i32 that is legal. I'd prefer that the comment explain this.

5471 ↗

(On Diff #162003)

I think all these things suggest us to set zeroext as default. Any thought?

I agree.

[LionNatsu]

Update patch, add comments about the special i1 argument type in LowerCall.

[LionNatsu]

Emmmm… sorry, what should I do next? I'm not familiar with the workflow of reviews.llvm.org…

[sylvestre.ledru]

You should ping the reviewer (if she/he is available on IRC).
Once you have the sign off, you ping the reviewer (or someone else like me) to land your patch in the vcs.

[hfinkel]

LGTM

[glaubitz]

Does this still need a review from cuviper?

[cuviper]

I don't think you need approval from all reviewers, as long as nobody actively disagrees. But FWIW, it looks good to me too.

[glaubitz]

Ok, thanks for the explanation. I wasn't sure how it's handled in LLVM. I'm in the OpenJDK upstream project and we always require two reviewers for changes to Hotspot!

[LionNatsu]

Thank you all very much. It seems all reviewers have now approved the revision. When will it be landed, just by the way? Did it miss the merge window (if there is one)? 😄

[glaubitz]

Has this been merged now? I don't see it in the github mirror yet.

[cuviper]

In D51108#1232917, @LionNatsu wrote:

Thank you all very much. It seems all reviewers have now approved the revision. When will it be landed, just by the way? Did it miss the merge window (if there is one)? 😄

You can either obtain commit access yourself, or ask someone to commit for you -- I can do so if you like.

Also see arcanist to help manage the review metadata.

[glaubitz]

or ask someone to commit for you -- I can do so if you like.

Yes, it would be great if you could commit that for me.

Also, could you commit https://reviews.llvm.org/D43146 as well which has also been accepted?

Thanks a lot!

[cuviper]

@glaubitz, I was specifically wondering if the author, @LionNatsu, would like to commit this on their own. I'm even more hesitant to intrude on your other review, where I wasn't involved at all.

[glaubitz]

@LionNatsu Could you push your commit yourself or do you need someone else to push it?

[LionNatsu]

Sorry for the late reply. Yeah, I think I can manage it.

Subversion commit worked, this PR closed by the commit automatically. (Sorry for the Git-ish terminologies). Frankly speaking, I learned svn just now.

BTW, I was a little bit confused by the various commit message format.

[glaubitz]

Great, thanks a lot! I'm also just learning how to contribute to LLVM as their setup is a bit unusual ;-).

llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp

if (VA.isRegLoc()) {
       if (ValVT == MVT::i1)
         ArgValue = DAG.getNode(ISD::TRUNCATE, dl, MVT::i1, ArgValue);
 
       InVals.push_back(ArgValue);
     } else {
       // 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);
       InVals.push_back(
           DAG.getLoad(VA.getValVT(), dl, Chain, FIN, MachinePointerInfo()));
     }
   }
 
if (Flags.isByVal()) {
       Chain = CallSeqStart = NewCallSeqStart;
 
       // Pass the address of the aggregate copy on the stack either in a
       // physical register or in the parameter list area of the current stack
       // frame to the callee.
       Arg = PtrOff;
     }
 
-    if (VA.isRegLoc()) {
-      if (Arg.getValueType() == MVT::i1)
-        Arg = DAG.getNode(ISD::ZERO_EXTEND, dl, MVT::i32, Arg);
+    // When useCRBits() is true, there can be i1 arguments.
+    // It is because getRegisterType(MVT::i1) => MVT::i1,
+    // and for other integer types getRegisterType() => MVT::i32.
+    // Extend i1 and ensure callee will get i32.
+      Arg = DAG.getNode(Flags.isSExt() ? ISD::SIGN_EXTEND : 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));
     } else {
       // Put argument in the parameter list area of the current stack frame.
       assert(VA.isMemLoc());
       unsigned LocMemOffset = VA.getLocMemOffset();
 

llvm/trunk/test/CodeGen/PowerPC/ppc32-i1-stack-arguments-abi-bug.ll

+; RUN: llc -verify-machineinstrs < %s -mcpu=ppc32 -mattr=+crbits | FileCheck %s
+target triple = "powerpc-unknown-linux-gnu"
+
+define void @check_callee(
+  i32, i32, i32, i32,
+  i32, i32, i32, i32,
+  i1 zeroext %s1
+) {
+  call void @check_caller(
+    i32 9, i32 9, i32 9, i32 9,
+    i32 9, i32 9, i32 9, i32 9,
+    i1 zeroext %s1)
+  ret void
+}
+
+; CHECK-LABEL: @check_callee
+; CHECK: lbz {{[0-9]+}}, 27(1)
+; CHECK: stw {{[0-9]+}}, 8(1)
+
+declare void @check_caller(
+  i32, i32, i32, i32,
+  i32, i32, i32, i32,
+  i1 zeroext
+)