Page MenuHomePhabricator

Support MS builtins using 'long' on LP64
ClosedPublic

Authored by bruno on Jun 19 2017, 6:43 PM.

Details

Summary

This allows for -fms-extensions to work the same on LP64. For example, _BitScanReverse is expected to be 32-bit, matching Windows/LLP64, even though long is 64-bit on x86_64 Darwin or Linux (LP64).

Implement this by adding a new character code 'N', which is 'int' if the target is LP64 and the same 'L' otherwise

Diff Detail

Repository
rL LLVM

Event Timeline

bruno created this revision.Jun 19 2017, 6:43 PM
bkelley accepted this revision.Jun 20 2017, 9:37 AM

Looks good to me. Thank you!

This revision is now accepted and ready to land.Jun 20 2017, 9:37 AM
majnemer added inline comments.
include/clang/Basic/Builtins.def
55 ↗(On Diff #103136)

Why not just LP64? Seems arbitrary to make this Darwin sensitive.

compnerd added inline comments.Jun 20 2017, 9:44 AM
lib/AST/ASTContext.cpp
8551 ↗(On Diff #103136)

I agree with @majnemer. Why not base this on the Int64Type?

rnk added inline comments.Jun 20 2017, 9:58 AM
include/clang/Basic/Builtins.def
55 ↗(On Diff #103136)

Every existing prefix is upper case. Do you think it makes it more readable to follow the pattern? Maybe it isn't worth it.

lib/AST/ASTContext.cpp
8551 ↗(On Diff #103136)

I'd suggest this code:

IsSpecialLong = true;
// Use "long" if is 32 bits. This prefix is used by intrinsics that need 32-bit types on LP64 platforms, but need to use "long" in the prototype on LLP64 platforms like Win64.
if (Context.getTargetInfo().getLongWidth() == 32)
  HowLong = 1;
break;
bruno added inline comments.Jun 20 2017, 1:44 PM
include/clang/Basic/Builtins.def
55 ↗(On Diff #103136)

At first attempt I tried to make it more generic, but there seems to be different expected behavior in other platforms: for instance, there are Linux/LP64 tests that expect 'long' here to be 64-bits, see CodeGen/ms-intrinsics-rotations.c.

55 ↗(On Diff #103136)

It might be better indeed, but seems like all "good letters" are taken, what about 'N'? As for "Narrow" long (Duncan's suggestion)?

lib/AST/ASTContext.cpp
8551 ↗(On Diff #103136)

See below.

8551 ↗(On Diff #103136)

I tried something similar before, but I get two tests failing CodeGen/ms-intrinsics-rotations.c and CodeGen/pr27892.c. Your suggestion hits the same failing tests. Both fails because of the Linux issue mentioned above: i32 codegen where i64 is expected. Of course I could improve the condition to handle Linux, but at that point I just thing it's better to use Darwin, which is what the fix is towards anyway. Additional ideas?

majnemer added inline comments.Jun 20 2017, 2:17 PM
lib/AST/ASTContext.cpp
8551 ↗(On Diff #103136)

I don't think we should sweep this under the rug just because there are some test failures. There is probably some latent bug worth investigating.

bruno added inline comments.Jun 20 2017, 2:25 PM
lib/AST/ASTContext.cpp
8551 ↗(On Diff #103136)

Well, there's specific testing for this behavior under Linux, so I assume someone needs this? I don't see how this is sweeping stuff under the rug.

rnk added inline comments.Jun 20 2017, 2:25 PM
lib/AST/ASTContext.cpp
8551 ↗(On Diff #103136)

I think I remember answer a question for Albert during his internship, and I said something like "they should stay longs", so he added those tests. Thinking about it now, those test should be changed to expect 32-bit ints.

bruno added inline comments.Jun 20 2017, 2:30 PM
lib/AST/ASTContext.cpp
8551 ↗(On Diff #103136)

Oh, I see. Gonna rework those then!

bruno updated this revision to Diff 103287.Jun 20 2017, 4:22 PM
bruno retitled this revision from Support MS builtins using 'long' on Darwin/LP64 to Support MS builtins using 'long' on LP64.
bruno edited the summary of this revision. (Show Details)

New patch addressing comments from reviewers.

rnk added inline comments.Jun 20 2017, 5:30 PM
include/clang/Basic/Builtins.def
55 ↗(On Diff #103287)

Fix the comment? "long" if target is LLP64, "int" otherwise?

test/CodeGen/ms-intrinsics-darwin.c
1 ↗(On Diff #103287)

I'd rather generalize test/CodeGen/ms-intrinsics.c to cover Darwin as well as Windows & Linux.

45 ↗(On Diff #103287)

You don't have a RUN: line for this.

bruno marked 3 inline comments as done.Jun 20 2017, 6:21 PM
bruno added inline comments.
test/CodeGen/ms-intrinsics-darwin.c
1 ↗(On Diff #103287)

ms-intrinsics.c currently includes intrin.h and does not cover linux. Instead I'm going to rename this ms-intrinsics-other.c, test for linux as well and cover the content from pr27892.c.

bruno updated this revision to Diff 103315.Jun 20 2017, 6:23 PM
bruno marked an inline comment as done.

Addressing comments!

rnk accepted this revision.Jun 20 2017, 6:24 PM

lgtm, thanks for shuffling all the tests around!

This revision was automatically updated to reflect the committed changes.