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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:5471
+    // Ensure callee will get either 0x00000001 or 0x00000000.
+    if (Arg.getValueType() == MVT::i1)
+      Arg = DAG.getNode(ISD::ZERO_EXTEND, dl, MVT::i32, Arg);
----------------
LionNatsu wrote:
> hfinkel wrote:
> > 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()?
> 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?
> 
> I think all these things suggest us to set zeroext as default. Any thought?

I agree.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:5471
+    // Ensure callee will get either 0x00000001 or 0x00000000.
+    if (Arg.getValueType() == MVT::i1)
+      Arg = DAG.getNode(Flags.isSExt() ? ISD::SIGN_EXTEND : ISD::ZERO_EXTEND,
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D51108