[PATCH] D81420: Fix size for _ExtInt types with builtins

This revision was automatically updated to reflect the committed changes. Closed by commit rG8799ebbc1f03: [clang] Fix or emit diagnostic for checked arithmetic builtins with _ExtInt… (authored by jtmott-intel, committed by erichkeane). Herald added a project: clang. Repository: rG LLVM

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel added a comment. I don't have commit access. For whoever performs the commit, here's a (draft) commit message: [clang] Fix or emit diagnostic for checked arithmetic builtins with _ExtInt types - Fix computed size for _ExtInt types passed to checked arithmetic builtins. -

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall accepted this revision. rjmccall added a comment. LGTM, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81420/new/ https://reviews.llvm.org/D81420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel marked an inline comment as done. jtmott-intel added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } rjmccall wrote: > jtmott-intel wrote: > > rjmccall wrote: > > > I know

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel updated this revision to Diff 270482. jtmott-intel added a comment. - Removed sign/unsign select. - Added test and support for placeholder types in builtins. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81420/new/ https://reviews.llvm.org/D81420 Files:

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } jtmott-intel wrote: > rjmccall wrote: > > I know this is existing code, but this is a broken mess. Please change > >

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } rjmccall wrote: > I know this is existing code, but this is a broken mess. Please change this > to: > > ``` >

[PATCH] D81420: Fix size for _ExtInt types with builtins

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Feel free to simplify the error message on commit. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7940 +def err_overflow_builtin_ext_int_max_size :

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/test/Sema/builtins-overflow.c:39 +_ExtInt(129) result; +_Bool status = __builtin_mul_overflow(x, y, ); // expected-error {{signed _ExtInt of bit sizes greater than 128 not supported}} + } jtmott-intel

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel updated this revision to Diff 269995. jtmott-intel added a comment. This updated patch prepared before John's latest comments. - Changed diagnostic message to something halfway between John and Erich's suggestions. - Removed superfluous calls to Arg.get. - Combined call to Diag and

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel marked 6 inline comments as done. jtmott-intel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939 "to a non-const integer (%0 invalid)">; +def err_overflow_builtin_extint_size : Error< + "_ExtInt argument larger than 64-bits

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } I know this is existing code, but this is a broken mess. Please change this to: ``` ExprResult Arg =

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel marked an inline comment as done. jtmott-intel added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:337 + QualType Ty = + I < 2 ? Arg.get()->getType() +: Arg.get()->getType()->getAs()->getPointeeType();

[PATCH] D81420: Fix size for _ExtInt types with builtins

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:330 + + // Disallow ExtIntType args larger than 128 bits to mul function until we + // improve backend support. Disallow _signed_ ... Comment at:

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939 "to a non-const integer (%0 invalid)">; +def err_overflow_builtin_extint_size : Error< + "_ExtInt argument larger than 64-bits to overflow builtin requires runtime "

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel updated this revision to Diff 269727. jtmott-intel added a comment. Limited diagnostic to *signed* _ExtInt, and added test to verify unsigned works. Reused existing diagnostic message rather than make a new one. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81420/new/

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel marked an inline comment as done. jtmott-intel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939 "to a non-const integer (%0 invalid)">; +def err_overflow_builtin_extint_size : Error< + "_ExtInt argument larger than 64-bits

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel updated this revision to Diff 269564. jtmott-intel added a comment. Updated the diagnostic check and message for 128 rather than 64 bits. Added sema tests for 128/129 sizes. Added codegen tests for 127/128 sizes to mul. CHANGES SINCE LAST ACTION

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel marked 2 inline comments as done. jtmott-intel added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1747-1748 break; case Builtin::BI__builtin_add_overflow: case Builtin::BI__builtin_sub_overflow: case Builtin::BI__builtin_mul_overflow:

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added a comment. Linking compiler-rt is something that should be automatically happening. It's possible that compiler-rt will have different maximum widths on different targets, though. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939 "to a

[PATCH] D81420: Fix size for _ExtInt types with builtins

lebedev.ri added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1747-1748 break; case Builtin::BI__builtin_add_overflow: case Builtin::BI__builtin_sub_overflow: case Builtin::BI__builtin_mul_overflow: I don't think this applies to

[PATCH] D81420: Fix size for _ExtInt types with builtins

erichkeane added a comment. So the idea of the SEMA check is right, but it ultimately isn't correct. Integers up to 128 can be called, as long as you use compiler-rt (--rtlib=compiler-rt), see https://godbolt.org/z/JDXZG_. However, 129 bits results in a crash in LLVM:

[PATCH] D81420: Fix size for _ExtInt types with builtins

erichkeane added a comment. Also, please file a bug (if you haven't already) about the >128 bit problem. We have a similar bug for normal division (https://bugs.llvm.org/show_bug.cgi?id=45649), but this needs one as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81420/new/

[PATCH] D81420: Fix size for _ExtInt types with builtins

jtmott-intel created this revision. jtmott-intel added reviewers: erichkeane, rjmccall. This patch addresses two issues. The first issue is use of the checked arithmetic builtins in combination with _ExtInt