[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[Mott, Jeffrey T via Phabricator via cfe-commits]

jtmott-intel added a comment.

Seems I don't have commit access. I'll look into it. For now, could someone 
push this commit? Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92892/new/

https://reviews.llvm.org/D92892

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[Mott, Jeffrey T via Phabricator via cfe-commits]

jtmott-intel updated this revision to Diff 314965.
jtmott-intel added a comment.

Updated comments to reflect "outside of" instead of "before".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92892/new/

https://reviews.llvm.org/D92892

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/CodeGen/object-size.c


Index: clang/test/CodeGen/object-size.c
===
--- clang/test/CodeGen/object-size.c
+++ clang/test/CodeGen/object-size.c
@@ -310,7 +310,7 @@
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
@@ -321,7 +321,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
@@ -337,7 +337,7 @@
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN([1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN([1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN([1].v[13], 2);
@@ -433,7 +433,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
@@ -518,7 +518,7 @@
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN([9].snd[0], 1);
 
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN([9].snd[0], 1);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11405,9 +11405,9 @@
   return false;
   }
 
-  // If we point to before the start of the object, there are no accessible
-  // bytes.
-  if (LVal.getLValueOffset().isNegative()) {
+  // If we point outside of the object, there are no accessible bytes.
+  if (LVal.getLValueOffset().isNegative() ||
+  ((Type & 1) && !LVal.Designator.isValidSubobject())) {
 Size = 0;
 return true;
   }


Index: clang/test/CodeGen/object-size.c
===
--- clang/test/CodeGen/object-size.c
+++ clang/test/CodeGen/object-size.c
@@ -310,7 +310,7 @@
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
@@ -321,7 +321,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
@@ -337,7 +337,7 @@
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN([1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN([1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN([1].v[13], 2);
@@ -433,7 +433,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
@@ -518,7 +518,7 @@
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN([9].snd[0], 1);
 
-  // CHECK: call i64 

[PATCH] D84049: Disable use of _ExtInt with '__atomic' builtins

[Mott, Jeffrey T via Phabricator via cfe-commits]

jtmott-intel added inline comments.



Comment at: libcxx/test/libcxx/atomics/ext-int.verify.cpp:1
+// REQUIRES: clang-11
+

ldionne wrote:
> This isn't great, since it won't run on clang-12, etc. I'll change it to:
> 
> ```
> // UNSUPPORTED: clang-4, clang-5, clang-6, clang-7, clang-8, clang-9, clang-10
> // UNSUPPORTED: apple-clang-9, apple-clang-10, apple-clang-11
> ```
> 
Thanks! That seems better.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84049/new/

https://reviews.llvm.org/D84049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84049: Disable use of _ExtInt with '__atomic' builtins

[Mott, Jeffrey T via Phabricator via cfe-commits]

jtmott-intel added a comment.

Committed to master, and created bug to cherry pick into 11.0.

https://bugs.llvm.org/show_bug.cgi?id=47222


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84049/new/

https://reviews.llvm.org/D84049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84049: Disable use of _ExtInt with '__atomic' builtins

[Mott, Jeffrey T via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGca77ab494aa2: Disable use of _ExtInt with 
__atomic builtins (authored by jtmott-intel).
Herald added projects: clang, libc++.
Herald added subscribers: libcxx-commits, cfe-commits.
Herald added a reviewer: libc++.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84049/new/

https://reviews.llvm.org/D84049

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/builtins.c
  clang/test/SemaCXX/ext-int.cpp
  libcxx/test/libcxx/atomics/ext-int.verify.cpp

Index: libcxx/test/libcxx/atomics/ext-int.verify.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/atomics/ext-int.verify.cpp
@@ -0,0 +1,11 @@
+// REQUIRES: clang-11
+
+#include 
+
+int main(int, char**)
+{
+  // expected-error@atomic:*1 {{_Atomic cannot be applied to integer type '_ExtInt(32)'}}
+  std::atomic<_ExtInt(32)> x {42};
+
+  return 0;
+}
Index: clang/test/SemaCXX/ext-int.cpp
===
--- clang/test/SemaCXX/ext-int.cpp
+++ clang/test/SemaCXX/ext-int.cpp
@@ -91,10 +91,11 @@
 _Complex _ExtInt(3) Cmplx;
 
 // Reject cases of _Atomic:
-// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(4)' with less than 1 byte of precision}}
+// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(4)'}}
 _Atomic _ExtInt(4) TooSmallAtomic;
-// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(9)' with a non power of 2 precision}}
+// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(9)'}}
 _Atomic _ExtInt(9) NotPow2Atomic;
+// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(128)'}}
 _Atomic _ExtInt(128) JustRightAtomic;
 
 // Test result types of Unary/Bitwise/Binary Operations:
Index: clang/test/Sema/builtins.c
===
--- clang/test/Sema/builtins.c
+++ clang/test/Sema/builtins.c
@@ -285,12 +285,16 @@
   __sync_fetch_and_add(ptr, value); // expected-error {{Atomic memory operand must have a power-of-two size}}
   // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
   __sync_nand_and_fetch(ptr, value); // expected-error {{Atomic memory operand must have a power-of-two size}}
+
+  __atomic_fetch_add(ptr, 1, 0); // expected-error {{argument to atomic builtin of type '_ExtInt' is not supported}}
 }
 
 void test_ei_i64i(_ExtInt(64) *ptr, int value) {
   __sync_fetch_and_add(ptr, value); // expect success
   // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
   __sync_nand_and_fetch(ptr, value); // expect success
+
+  __atomic_fetch_add(ptr, 1, 0); // expected-error {{argument to atomic builtin of type '_ExtInt' is not supported}}
 }
 
 void test_ei_ii42(int *ptr, _ExtInt(42) value) {
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -8963,11 +8963,8 @@
 else if (!T.isTriviallyCopyableType(Context))
   // Some other non-trivially-copyable type (probably a C++ class)
   DisallowedKind = 7;
-else if (auto *ExtTy = T->getAs()) {
-  if (ExtTy->getNumBits() < 8)
+else if (T->isExtIntType()) {
 DisallowedKind = 8;
-  else if (!llvm::isPowerOf2_32(ExtTy->getNumBits()))
-DisallowedKind = 9;
 }
 
 if (DisallowedKind != -1) {
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5050,6 +5050,11 @@
 ? 0
 : 1);
 
+  if (ValType->isExtIntType()) {
+Diag(Ptr->getExprLoc(), diag::err_atomic_builtin_ext_int_prohibit);
+return ExprError();
+  }
+
   return AE;
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6038,9 +6038,8 @@
 def err_atomic_specifier_bad_type
 : Error<"_Atomic cannot be applied to "
 "%select{incomplete |array |function |reference |atomic |qualified "
-"|sizeless ||integer |integer }0type "
-"%1 %select{|||which is not trivially copyable|with less than "
-"1 byte of precision|with a non power of 2 precision}0">;
+"|sizeless ||integer }0type "
+"%1 %select{|||which is not trivially copyable|}0">;
 
 // Expressions.
 def ext_sizeof_alignof_function_type : Extension<
@@ -7967,6 +7966,8 @@
   " 1,2,4 or 8 byte type (%0 

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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.
  - Emit diagnostic when signed _ExtInt larger than 128-bits is passed to 
__builtin_mul_overflow.
  - Change Sema checks for builtins to accept placeholder types.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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 this is existing code, but this is a broken mess.  Please change 
> > > this to:
> > > 
> > > ```
> > >   ExprResult Arg = 
> > > DefaultFunctionArrayLvalueConversion(TheCall->getArg(2));
> > >   if (Arg.isInvalid()) return true;
> > >   TheCall->setArg(2, Arg.get());
> > > 
> > >   QualType Ty = Arg.get()->getType();
> > >   const auto *PtrTy = Ty->getAs();
> > >   if (!PtrTy ||
> > >   !PtrTy->getPointeeType()->isIntegerType() ||
> > >   PtrTy->getPointeeType().isConstQualified()) {
> > > S.Diag(Arg.get()->getBeginLoc(),
> > >diag::err_overflow_builtin_must_be_ptr_int)
> > >   << Ty << Arg.get()->getSourceRange();
> > > return true;
> > >   }
> > > ```
> > > 
> > > Test case would be something like (in ObjC):
> > > 
> > > ```
> > > @interface HasPointer
> > > @property int *pointer;
> > > @end
> > > 
> > > void test(HasPointer *hp) {
> > >   __builtin_add_overflow(x, y, hp.pointer);
> > > }
> > > ```
> > > 
> > > And the earlier block needs essentially the same change (but obviously 
> > > checking for a different type).  You can't check types before doing 
> > > placeholder conversions, and once you do l-value conversions and a 
> > > type-check, you really don't need the full parameter-initialization logic.
> > I've implemented this locally but I have a quick question about the test. 
> > It passed even before I applied this code change. Is this test expected to 
> > fail before this change and pass after? Or is it just to prove that the 
> > feature (placeholder argument types?) works in general?
> Oh, we seem to handle placeholders specially, nevermind.  Well, what I said 
> about doing conversions before checking the type still holds, but the test 
> case is just this:
> 
> ```
> int z[1];
> __builtin_add_overflow(x, y, z);
> ```
> 
> where we should decay the argument to a pointer instead of rejecting it.
Done. Latest updated patch includes these code changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-overflow.c
  clang/test/Sema/builtins-overflow.c
  clang/test/Sema/builtins-overflow.m

Index: clang/test/Sema/builtins-overflow.m
===
--- /dev/null
+++ clang/test/Sema/builtins-overflow.m
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+void test() {
+  int z[1];
+  __builtin_add_overflow(1, 1, z);
+}
Index: clang/test/Sema/builtins-overflow.c
===
--- clang/test/Sema/builtins-overflow.c
+++ clang/test/Sema/builtins-overflow.c
@@ -19,4 +19,23 @@
   __builtin_add_overflow(1, 1, 3);  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('int' invalid)}}
   __builtin_add_overflow(1, 1, );  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('float *' invalid)}}
   __builtin_add_overflow(1, 1, );  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('const unsigned int *' invalid)}}
+
+  {
+_ExtInt(128) x = 1;
+_ExtInt(128) y = 1;
+_ExtInt(128) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expect ok
+  }
+  {
+unsigned _ExtInt(129) x = 1;
+unsigned _ExtInt(129) y = 1;
+unsigned _ExtInt(129) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expect ok
+  }
+  {
+_ExtInt(129) x = 1;
+_ExtInt(129) y = 1;
+_ExtInt(129) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expected-error {{__builtin_mul_overflow does not support signed _ExtInt operands of more than 128 bits}}
+  }
 }
Index: clang/test/CodeGen/builtins-overflow.c
===
--- clang/test/CodeGen/builtins-overflow.c
+++ clang/test/CodeGen/builtins-overflow.c
@@ -41,6 +41,20 @@
   return r;
 }
 
+int test_add_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_add_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.sadd.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_add_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_sub_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -69,6 +83,20 @@
   return r;
 }
 
+int test_sub_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.ssub.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_sub_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_mul_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -97,6 +125,48 @@
   return r;
 }
 
+int test_mul_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.smul.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_mul_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
+int test_mul_overflow_xint127_xint127_xint127(_ExtInt(127) x, _ExtInt(127) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint127_xint127_xint127({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i127, i1 } @llvm.smul.with.overflow.i127(i127 %{{.+}}, i127 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i127, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i127, i1 } [[S]], 0
+  // CHECK: store i127 [[Q]], i127*
+  // CHECK: br i1 [[C]]
+  _ExtInt(127) r;
+  if 

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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:
> 
> ```
>   ExprResult Arg = DefaultFunctionArrayLvalueConversion(TheCall->getArg(2));
>   if (Arg.isInvalid()) return true;
>   TheCall->setArg(2, Arg.get());
> 
>   QualType Ty = Arg.get()->getType();
>   const auto *PtrTy = Ty->getAs();
>   if (!PtrTy ||
>   !PtrTy->getPointeeType()->isIntegerType() ||
>   PtrTy->getPointeeType().isConstQualified()) {
> S.Diag(Arg.get()->getBeginLoc(),
>diag::err_overflow_builtin_must_be_ptr_int)
>   << Ty << Arg.get()->getSourceRange();
> return true;
>   }
> ```
> 
> Test case would be something like (in ObjC):
> 
> ```
> @interface HasPointer
> @property int *pointer;
> @end
> 
> void test(HasPointer *hp) {
>   __builtin_add_overflow(x, y, hp.pointer);
> }
> ```
> 
> And the earlier block needs essentially the same change (but obviously 
> checking for a different type).  You can't check types before doing 
> placeholder conversions, and once you do l-value conversions and a 
> type-check, you really don't need the full parameter-initialization logic.
I've implemented this locally but I have a quick question about the test. It 
passed even before I applied this code change. Is this test expected to fail 
before this change and pass after? Or is it just to prove that the feature 
(placeholder argument types?) works in general?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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 return of error status.
- Simplified but kept ternary (for now).
- Updated comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-overflow.c
  clang/test/Sema/builtins-overflow.c

Index: clang/test/Sema/builtins-overflow.c
===
--- clang/test/Sema/builtins-overflow.c
+++ clang/test/Sema/builtins-overflow.c
@@ -19,4 +19,23 @@
   __builtin_add_overflow(1, 1, 3);  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('int' invalid)}}
   __builtin_add_overflow(1, 1, );  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('float *' invalid)}}
   __builtin_add_overflow(1, 1, );  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('const unsigned int *' invalid)}}
+
+  {
+_ExtInt(128) x = 1;
+_ExtInt(128) y = 1;
+_ExtInt(128) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expect ok
+  }
+  {
+unsigned _ExtInt(129) x = 1;
+unsigned _ExtInt(129) y = 1;
+unsigned _ExtInt(129) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expect ok
+  }
+  {
+_ExtInt(129) x = 1;
+_ExtInt(129) y = 1;
+_ExtInt(129) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expected-error {{__builtin_mul_overflow does not support signed _ExtInt operands of more than 128 bits}}
+  }
 }
Index: clang/test/CodeGen/builtins-overflow.c
===
--- clang/test/CodeGen/builtins-overflow.c
+++ clang/test/CodeGen/builtins-overflow.c
@@ -41,6 +41,20 @@
   return r;
 }
 
+int test_add_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_add_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.sadd.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_add_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_sub_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -69,6 +83,20 @@
   return r;
 }
 
+int test_sub_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.ssub.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_sub_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_mul_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -97,6 +125,48 @@
   return r;
 }
 
+int test_mul_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.smul.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_mul_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
+int test_mul_overflow_xint127_xint127_xint127(_ExtInt(127) x, _ExtInt(127) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint127_xint127_xint127({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i127, i1 } @llvm.smul.with.overflow.i127(i127 %{{.+}}, i127 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i127, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i127, i1 } [[S]], 0
+  // CHECK: store i127 [[Q]], i127*
+  // CHECK: br i1 [[C]]
+  _ExtInt(127) r;
+  if (__builtin_mul_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
+int test_mul_overflow_xint128_xint128_xint128(_ExtInt(128) x, _ExtInt(128) y) {
+  // 

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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 to overflow builtin requires runtime "

rjmccall wrote:
> jtmott-intel wrote:
> > rjmccall wrote:
> > > erichkeane wrote:
> > > > Mentioning target-specific support here seems incorrect. @rjmccall I 
> > > > cannot think of a better wording, can you?
> > > "overflow builtins do not support _ExtInt operands of more than %0 bits 
> > > on this target"?  I don't think it's unreasonable to mention the 
> > > target-specificness of it.   Hard-coding the number 64 in the diagnostic 
> > > seems excessively targeted, though.
> > I discovered there's a good existing message I could use that already takes 
> > the bitwidth as a parameter. I decided not to add a new one. 
> > Thoughts/preferences? Here's how the message would come out.
> > 
> > test.c:5:43: error: signed _ExtInt of bit sizes greater than 128 not 
> > supported
> > _Bool status = __builtin_mul_overflow(x, y, );
> >   ^
> > 1 error generated.
> > 
> It'd be much clearer to say something about the fact that it's just this 
> builtin that's not supported.
Updated message to something halfway between John and Erich's suggestions. 
Current result:

test.c:5:43: error: __builtin_mul_overflow does not support signed _ExtInt 
operands of more than 128 bits
_Bool status = __builtin_mul_overflow(x, y, );
  ^
1 error generated.





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}}
+  }

erichkeane wrote:
> As @rjmccall said, I'd suggest the new message anyway that mentions the 
> builtin, otherwise this would be confusing for me.  Something like:
> 
> signed argument to __builtin_mul_overflow must have a bitwidth of less than 
> or equal to 128.
Updated message to something halfway between your and John's suggestions. 
Current result:

test.c:5:43: error: __builtin_mul_overflow does not support signed _ExtInt 
operands of more than 128 bits
_Bool status = __builtin_mul_overflow(x, y, );
  ^
1 error generated.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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();

erichkeane wrote:
> Try:
> 
> Type *Ty = ArgExpr->getType()->getPointeeOrArrayElementType();
> 
> instead of the ternary.
A complication I'm seeing is that `getPointeeOrArrayElementType` returns a 
`Type` but getIntWidth wants a `QualType`. Some options that occurred to me:

- Is it acceptable to just wrap a `Type` in a `QualType`?
- I might be able to add a `getIntWidth(const Type*)` overload.
- Looks like I can skip the `getAs()` and just call 
`getPointeeType` directly. The line would be shorter but I would still need the 
ternary.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D81420

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-overflow.c
  clang/test/Sema/builtins-overflow.c

Index: clang/test/Sema/builtins-overflow.c
===
--- clang/test/Sema/builtins-overflow.c
+++ clang/test/Sema/builtins-overflow.c
@@ -19,4 +19,23 @@
   __builtin_add_overflow(1, 1, 3);  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('int' invalid)}}
   __builtin_add_overflow(1, 1, );  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('float *' invalid)}}
   __builtin_add_overflow(1, 1, );  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('const unsigned int *' invalid)}}
+
+  {
+_ExtInt(128) x = 1;
+_ExtInt(128) y = 1;
+_ExtInt(128) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expect ok
+  }
+  {
+unsigned _ExtInt(129) x = 1;
+unsigned _ExtInt(129) y = 1;
+unsigned _ExtInt(129) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expect ok
+  }
+  {
+_ExtInt(129) x = 1;
+_ExtInt(129) y = 1;
+_ExtInt(129) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expected-error {{signed _ExtInt of bit sizes greater than 128 not supported}}
+  }
 }
Index: clang/test/CodeGen/builtins-overflow.c
===
--- clang/test/CodeGen/builtins-overflow.c
+++ clang/test/CodeGen/builtins-overflow.c
@@ -41,6 +41,20 @@
   return r;
 }
 
+int test_add_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_add_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.sadd.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_add_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_sub_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -69,6 +83,20 @@
   return r;
 }
 
+int test_sub_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.ssub.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_sub_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_mul_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -97,6 +125,48 @@
   return r;
 }
 
+int test_mul_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.smul.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_mul_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
+int test_mul_overflow_xint127_xint127_xint127(_ExtInt(127) x, _ExtInt(127) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint127_xint127_xint127({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i127, i1 } @llvm.smul.with.overflow.i127(i127 %{{.+}}, i127 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i127, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i127, i1 } [[S]], 0
+  // CHECK: store i127 [[Q]], i127*
+  // CHECK: br i1 [[C]]
+  _ExtInt(127) r;
+  if (__builtin_mul_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
+int test_mul_overflow_xint128_xint128_xint128(_ExtInt(128) x, _ExtInt(128) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint128_xint128_xint128({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i128, i1 } @llvm.smul.with.overflow.i128(i128 %{{.+}}, i128 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = 

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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 to overflow builtin requires runtime "

rjmccall wrote:
> erichkeane wrote:
> > Mentioning target-specific support here seems incorrect. @rjmccall I cannot 
> > think of a better wording, can you?
> "overflow builtins do not support _ExtInt operands of more than %0 bits on 
> this target"?  I don't think it's unreasonable to mention the 
> target-specificness of it.   Hard-coding the number 64 in the diagnostic 
> seems excessively targeted, though.
I discovered there's a good existing message I could use that already takes the 
bitwidth as a parameter. I decided not to add a new one. Thoughts/preferences? 
Here's how the message would come out.

test.c:5:43: error: signed _ExtInt of bit sizes greater than 128 not 
supported
_Bool status = __builtin_mul_overflow(x, y, );
  ^
1 error generated.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-overflow.c
  clang/test/Sema/builtins-overflow.c

Index: clang/test/Sema/builtins-overflow.c
===
--- clang/test/Sema/builtins-overflow.c
+++ clang/test/Sema/builtins-overflow.c
@@ -19,4 +19,17 @@
   __builtin_add_overflow(1, 1, 3);  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('int' invalid)}}
   __builtin_add_overflow(1, 1, );  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('float *' invalid)}}
   __builtin_add_overflow(1, 1, );  // expected-error {{result argument to overflow builtin must be a pointer to a non-const integer ('const unsigned int *' invalid)}}
+
+  {
+_ExtInt(128) x = 1;
+_ExtInt(128) y = 1;
+_ExtInt(128) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expect ok
+  }
+  {
+_ExtInt(129) x = 1;
+_ExtInt(129) y = 1;
+_ExtInt(129) result;
+_Bool status = __builtin_mul_overflow(x, y, ); // expected-error {{_ExtInt argument larger than 128-bits to overflow builtin requires runtime support that is not available for this target}}
+  }
 }
Index: clang/test/CodeGen/builtins-overflow.c
===
--- clang/test/CodeGen/builtins-overflow.c
+++ clang/test/CodeGen/builtins-overflow.c
@@ -41,6 +41,20 @@
   return r;
 }
 
+int test_add_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_add_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.sadd.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_add_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_sub_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -69,6 +83,20 @@
   return r;
 }
 
+int test_sub_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.ssub.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_sub_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_mul_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -97,6 +125,48 @@
   return r;
 }
 
+int test_mul_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.smul.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_mul_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
+int test_mul_overflow_xint127_xint127_xint127(_ExtInt(127) x, _ExtInt(127) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint127_xint127_xint127({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i127, i1 } @llvm.smul.with.overflow.i127(i127 %{{.+}}, i127 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i127, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i127, i1 } [[S]], 0
+  // CHECK: store i127 [[Q]], i127*
+  // CHECK: br i1 [[C]]
+  _ExtInt(127) r;
+  if (__builtin_mul_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
+int test_mul_overflow_xint128_xint128_xint128(_ExtInt(128) x, _ExtInt(128) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint128_xint128_xint128({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i128, i1 } @llvm.smul.with.overflow.i128(i128 %{{.+}}, i128 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i128, i1 } [[S]], 1
+  // CHECK-DAG: 

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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:

lebedev.ri wrote:
> I don't think this applies to `add`/`sub`: https://godbolt.org/z/Dj3GLP (or 
> at least not all of them?)
> OTOH this does apply to plain division/remainder: https://godbolt.org/z/SXNw8H
Correct. I passed the `BuiltinID` so that in `SemaBuiltinOverflow` I could 
check for `mul` specifically. Alternatively, I could give `mul` its own case 
"block" separate from `add`/`sub`. Thoughts/preferences?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mott, Jeffrey T via Phabricator via cfe-commits]

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 
 types. _ExtInt 
types with non-power of two sizes would be (inconsistently) rounded up to the 
next largest power of two. The inconsistency caused the following compile error:

  $ cat test.c
  int main(void) {
  _ExtInt(31) x = 1;
  _ExtInt(31) y = 1;
  _ExtInt(31) result;
  _Bool status = __builtin_add_overflow(x, y, );
  return 0;
  }
  
  $ clang test.c
  clang-11: 
/home/user/llvm_workspace/llvm-project/llvm/lib/IR/Instructions.cpp:1408: void 
llvm::StoreInst::AssertOK(): Assertion `getOperand(0)->getType() == 
cast(getOperand(1)->getType())->getElementType() && "Ptr must be a 
pointer to Val type!"' failed.

To fix this, I added the above example as a failing test to 
`clang/test/CodeGen/builtins-overflow.c`, and I changed 
`clang/lib/CodeGen/CGBuiltin.cpp` to get the correct size for _ExtInt types and 
allow the new test to pass.

The second issue is with `__builtin_mul_overflow` specifically in combination 
with _ExtInt types larger than 64 bits. For that combination, the emitted IR 
appears to be correct, but a full compile caused the following link error (for 
at least X86 target):

  $ cat test.c
  int main(void) {
  _ExtInt(65) x = 1;
  _ExtInt(65) y = 1;
  _ExtInt(65) result;
  _Bool status = __builtin_mul_overflow(x, y, );
  return 0;
  }
  $ clang test.c
  /tmp/test-2b8174.o: In function `main':
  test.c:(.text+0x63): undefined reference to `__muloti4'
  clang-11: error: linker command failed with exit code 1 (use -v to see 
invocation)

To address this -- for now -- I changed `clang/lib/Sema/SemaChecking.cpp` to 
emit a diagnostic that explains the limitation to users better than the link 
error does. I did my best to borrow verbiage from similar messages. The new 
compile result is:

  $ clang test.c
  test.c:5:43: error: _ExtInt argument larger than 64-bits to overflow builtin 
requires runtime support that is not available for this target
  _Bool status = __builtin_mul_overflow(x, y, );
^
  1 error generated.


https://reviews.llvm.org/D81420

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtins-overflow.c

Index: clang/test/CodeGen/builtins-overflow.c
===
--- clang/test/CodeGen/builtins-overflow.c
+++ clang/test/CodeGen/builtins-overflow.c
@@ -41,6 +41,20 @@
   return r;
 }
 
+int test_add_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_add_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.sadd.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_add_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_sub_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -69,6 +83,20 @@
   return r;
 }
 
+int test_sub_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_sub_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.ssub.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_sub_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 unsigned test_mul_overflow_uint_uint_uint(unsigned x, unsigned y) {
   // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_uint_uint_uint
   // CHECK-NOT: ext
@@ -97,6 +125,20 @@
   return r;
 }
 
+int test_mul_overflow_xint31_xint31_xint31(_ExtInt(31) x, _ExtInt(31) y) {
+  // CHECK-LABEL: define {{(dso_local )?}}i32 @test_mul_overflow_xint31_xint31_xint31({{.+}})
+  // CHECK-NOT: ext
+  // CHECK: [[S:%.+]] = call { i31, i1 } @llvm.smul.with.overflow.i31(i31 %{{.+}}, i31 %{{.+}})
+  // CHECK-DAG: [[C:%.+]] = extractvalue { i31, i1 } [[S]], 1
+  // CHECK-DAG: [[Q:%.+]] = extractvalue { i31, i1 } [[S]], 0
+  // CHECK: store i31 [[Q]], i31*
+  // CHECK: br i1 [[C]]
+  _ExtInt(31) r;
+  if (__builtin_mul_overflow(x, y, ))
+overflowed();
+  return r;
+}
+
 int