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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits