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

[Erich Keane via Phabricator via cfe-commits]

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: clang/lib/Sema/SemaChecking.cpp:334
+    for (unsigned I = 0; I < 3; ++I) {
+      ExprResult Arg = TheCall->getArg(I);
+      // Third argument will be a pointer
----------------
Since you're always doing "Arg.get" below, this should probably just store the Expr*.  




================
Comment at: clang/lib/Sema/SemaChecking.cpp:335
+      ExprResult Arg = TheCall->getArg(I);
+      // Third argument will be a pointer
+      QualType Ty =
----------------
Comments must end in a period.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:337
+      QualType Ty =
+          I < 2 ? Arg.get()->getType()
+                : Arg.get()->getType()->getAs<PointerType>()->getPointeeType();
----------------
Try:

Type *Ty = ArgExpr->getType()->getPointeeOrArrayElementType();

instead of the ternary.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:340
+      if (Ty->isExtIntType() && Ty->isSignedIntegerType() &&
+          S.getASTContext().getIntWidth(Ty) > 128) {
+        S.Diag(Arg.get()->getBeginLoc(), diag::err_ext_int_max_size)
----------------
remove the curley brackets, and make this: 

return S.Diag(....


================
Comment at: clang/test/Sema/builtins-overflow.c:39
+    _ExtInt(129) result;
+    _Bool status = __builtin_mul_overflow(x, y, &result); // expected-error {{signed _ExtInt of bit sizes greater than 128 not supported}}
+  }
----------------
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.


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

https://reviews.llvm.org/D81420