[PATCH] D34377: Support MS builtins using 'long' on Darwin/LP64
Bruno Cardoso Lopes via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 20 13:44:18 PDT 2017
bruno added inline comments.
================
Comment at: include/clang/Basic/Builtins.def:55
// W -> int64_t
+// l -> 'int' if builtin is a MS extensions and the target is Darwin/LP64.
+// Defaults to 'L' otherwise.
----------------
rnk wrote:
> majnemer wrote:
> > Why not just LP64? Seems arbitrary to make this Darwin sensitive.
> Every existing prefix is upper case. Do you think it makes it more readable to follow the pattern? Maybe it isn't worth it.
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.
================
Comment at: include/clang/Basic/Builtins.def:55
// W -> int64_t
+// l -> 'int' if builtin is a MS extensions and the target is Darwin/LP64.
+// Defaults to 'L' otherwise.
----------------
bruno wrote:
> rnk wrote:
> > majnemer wrote:
> > > Why not just LP64? Seems arbitrary to make this Darwin sensitive.
> > Every existing prefix is upper case. Do you think it makes it more readable to follow the pattern? Maybe it isn't worth it.
> 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.
It might be better indeed, but seems like all "good letters" are taken, what about 'N'? As for "Narrow" long (Duncan's suggestion)?
================
Comment at: lib/AST/ASTContext.cpp:8551
+ break;
+ }
case 'W':
----------------
rnk wrote:
> compnerd wrote:
> > I agree with @majnemer. Why not base this on the Int64Type?
> 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;
See below.
================
Comment at: lib/AST/ASTContext.cpp:8551
+ break;
+ }
case 'W':
----------------
bruno wrote:
> rnk wrote:
> > compnerd wrote:
> > > I agree with @majnemer. Why not base this on the Int64Type?
> > 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;
> See below.
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?
https://reviews.llvm.org/D34377
More information about the cfe-commits
mailing list