This is an archive of the discontinued LLVM Phabricator instance.

Allow __ieee128 as an alias to __float128 on ppc
ClosedPublic

Authored by serge-sans-paille on Mar 3 2021, 3:02 AM.

Details

Summary

This matches gcc behavior

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Mar 3 2021, 3:02 AM
serge-sans-paille added a reviewer: qshanz.
qiucf edited reviewers, added: steven.zhang, Restricted Project, hubert.reinterpretcast, nemanjai, qiucf; removed: qshanz.Mar 3 2021, 6:22 PM
qiucf added a comment.Mar 3 2021, 6:26 PM

Hi, you mentioned that this patch aliases __ieee128 to __float128 on PPC. But this seems a target-independent change, right?

The change itself looks reasonable since libstdcxx may refer to __ieee128 type.

nemanjai added a subscriber: rsmith.Mar 5 2021, 2:57 AM

Hi, you mentioned that this patch aliases __ieee128 to __float128 on PPC. But this seems a target-independent change, right?

The change itself looks reasonable since libstdcxx may refer to __ieee128 type.

I agree that this looks fine as long as the title is fixed up - it is not target specific and it is also not __ieee18 but __ieee128. Since it is target independent, you may want @rsmith to have a look as well.

GCC only defines __ieee128 for 64-bit power, and libstdc++ only uses it for 64-bit power if the C library supports it.

serge-sans-paille retitled this revision from Allow __ieee18 as an alias to __float128 on ppc to Allow __ieee128 as an alias to __float128 on ppc.

Update description, and update implementation to more closely match gcc behavior.

This should be ppc specific now.

@qiucf does it look good now?

qiucf added a comment.Mar 11 2021, 8:00 AM

@qiucf does it look good now?

Thanks. This looks fine to me. See if there're any comments from other reviewers.

GCC doesn't seem to implement this as a keyword?

namespace Q { int __ieee128; } // compiles

GCC doesn't seem to implement this as a keyword?

namespace Q { int __ieee128; } // compiles

Correct. That being said, using an identifier starting with __ is UB in C and C++, so I guess it's not critical (?). I'm open to suggestion on how to implement that behavior in clang though :-)

jwakely added a comment.EditedMar 13 2021, 12:14 AM

GCC doesn't seem to implement this as a keyword?

namespace Q { int __ieee128; } // compiles

That seems to be true for __float128 too, even on x86_64. I'll find out what's going on there.

Edit: and also __float80 on x86. Maybe a bug (or "feature") of target-specific built-in types in GCC. I'll find out on Monday.

As Serge said, it's UB to use that as an identifier, so not a show stopper, and Clang and GCC already diverge here for existing types.

I'm open to suggestion on how to implement that behavior in clang though :-)

My suggestion would be "don't", because GCC's behaviour doesn't make sense!

My suggestion would be "don't", because GCC's behaviour doesn't make sense!

@qiucf based on @jwakely feedback, I'm going to leave the implementation as is :-)

This revision is now accepted and ready to land.Mar 15 2021, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 10:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
serge-sans-paille added a comment.EditedMar 15 2021, 10:29 AM

Thanks @hubert.reinterpretcast , @jwakely and @qiucf for the feedback on that review!