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

Authored by LionNatsu on Aug 22 2018, 9:40 AM.

Details

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

Diff Detail

Repository
rL LLVM
LionNatsu created this revision.Aug 22 2018, 9:40 AM
LionNatsu edited the summary of this revision. (Show Details)Aug 22 2018, 9:56 AM
LionNatsu edited the summary of this revision. (Show Details)
nagisa added a subscriber: nagisa.Aug 22 2018, 9:59 AM

Please add a test. See this.

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

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

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 added reviewers: hfinkel, cuviper.

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

@nagisa Thank you for your help! :)

hfinkel added inline comments.Aug 23 2018, 1:52 PM
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 added inline comments.Aug 23 2018, 8:49 PM
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 retitled this revision from [PowerPC] Fix wrong ABI for i1 stack arguments for PPC32 to [PowerPC] Fix wrong ABI for i1 stack arguments on PPC32.
LionNatsu edited the summary of this revision. (Show Details)

Use SIGN_EXTEND in LowerCall_32SVR4 if explicitly specified, update summary.

LionNatsu marked 2 inline comments as done.Aug 23 2018, 11:30 PM
hfinkel added inline comments.Sat, Aug 25, 2:52 PM
lib/Target/PowerPC/PPCISelLowering.cpp
5471 ↗(On Diff #162003)

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

I agree.

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.

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

LionNatsu marked 2 inline comments as done.Thu, Aug 30, 11:19 AM

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

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 accepted this revision.Tue, Sep 11, 8:26 AM

LGTM

This revision is now accepted and ready to land.Tue, Sep 11, 8:26 AM

Does this still need a review from 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.

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!

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)? 😄

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

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.

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!

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

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

This revision was automatically updated to reflect the committed changes.

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.

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