[PATCH] D34377: Support MS builtins using 'long' on Darwin/LP64
bruno added inline comments. Comment at: lib/AST/ASTContext.cpp:8551 + break; +} case 'W': bruno wrote: > rnk wrote: > > majnemer wrote: > > > bruno wrote: > > > > 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? > > > 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. > > 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. > 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. --- Oh, I see. Gonna rework those then! https://reviews.llvm.org/D34377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34377: Support MS builtins using 'long' on Darwin/LP64
rnk added inline comments. Comment at: lib/AST/ASTContext.cpp:8551 + break; +} case 'W': majnemer wrote: > bruno wrote: > > 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? > 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. 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. https://reviews.llvm.org/D34377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34377: Support MS builtins using 'long' on Darwin/LP64
bruno added inline comments. Comment at: lib/AST/ASTContext.cpp:8551 + break; +} case 'W': majnemer wrote: > bruno wrote: > > 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? > 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. 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. https://reviews.llvm.org/D34377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34377: Support MS builtins using 'long' on Darwin/LP64
majnemer added inline comments. Comment at: lib/AST/ASTContext.cpp:8551 + break; +} case 'W': bruno wrote: > 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? 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. https://reviews.llvm.org/D34377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34377: Support MS builtins using 'long' on Darwin/LP64
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
[PATCH] D34377: Support MS builtins using 'long' on Darwin/LP64
rnk 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. 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. Comment at: lib/AST/ASTContext.cpp:8551 + break; +} case 'W': 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; https://reviews.llvm.org/D34377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
