[PATCH] D52873: Remove unwanted signedness conversion from tests

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC344778: [OpenCL] Remove unwanted signedness conversion from 
tests (authored by mantognini, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D52873

Files:
  test/SemaOpenCL/cl20-device-side-enqueue.cl


Index: test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -212,15 +212,15 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : enable
 
-kernel void foo(global int *buf)
+kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // 
expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', 
expected 'ndrange_t' argument type}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // 
expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', 
expected block argument type}}
 }
 
-kernel void bar(global int *buf)
+kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
@@ -230,13 +230,13 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : disable
 
-kernel void foo1(global int *buf)
+kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // 
expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' 
requires cl_khr_subgroups extension to be enabled}}
 }
 
-kernel void bar1(global int *buf)
+kernel void bar1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-error 
{{use of declaration 'get_kernel_sub_group_count_for_ndrange' requires 
cl_khr_subgroups extension to be enabled}}


Index: test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -212,15 +212,15 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : enable
 
-kernel void foo(global int *buf)
+kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected block argument type}}
 }
 
-kernel void bar(global int *buf)
+kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
@@ -230,13 +230,13 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : disable
 
-kernel void foo1(global int *buf)
+kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' requires cl_khr_subgroups extension to be enabled}}
 }
 
-kernel void bar1(global int *buf)
+kernel void bar1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_sub_group_count_for_ndrange' requires cl_khr_subgroups extension to be enabled}}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52873: Remove unwanted signedness conversion from tests

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL344778: [OpenCL] Remove unwanted signedness conversion from 
tests (authored by mantognini, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52873?vs=168256&id=170165#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52873

Files:
  cfe/trunk/test/SemaOpenCL/cl20-device-side-enqueue.cl


Index: cfe/trunk/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- cfe/trunk/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ cfe/trunk/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -212,15 +212,15 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : enable
 
-kernel void foo(global int *buf)
+kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // 
expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', 
expected 'ndrange_t' argument type}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // 
expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', 
expected block argument type}}
 }
 
-kernel void bar(global int *buf)
+kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
@@ -230,13 +230,13 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : disable
 
-kernel void foo1(global int *buf)
+kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // 
expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' 
requires cl_khr_subgroups extension to be enabled}}
 }
 
-kernel void bar1(global int *buf)
+kernel void bar1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-error 
{{use of declaration 'get_kernel_sub_group_count_for_ndrange' requires 
cl_khr_subgroups extension to be enabled}}


Index: cfe/trunk/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- cfe/trunk/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ cfe/trunk/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -212,15 +212,15 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : enable
 
-kernel void foo(global int *buf)
+kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected block argument type}}
 }
 
-kernel void bar(global int *buf)
+kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
@@ -230,13 +230,13 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : disable
 
-kernel void foo1(global int *buf)
+kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' requires cl_khr_subgroups extension to be enabled}}
 }
 
-kernel void bar1(global int *buf)
+kernel void bar1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_sub_group_count_for_ndrange' requires cl_khr_subgroups extension to be enabled}}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52875: Fix definitions of __builtin_(add|sub|mul)_overflow

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC344891: [OpenCL] Fix definitions of 
__builtin_(add|sub|mul)_overflow (authored by mantognini, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D52875

Files:
  include/clang/Basic/Builtins.def


Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -1398,9 +1398,9 @@
 BUILTIN(__builtin_subcll, "ULLiULLiCULLiCULLiCULLi*", "n")
 
 // Checked Arithmetic Builtins for Security.
-BUILTIN(__builtin_add_overflow, "v.", "nt")
-BUILTIN(__builtin_sub_overflow, "v.", "nt")
-BUILTIN(__builtin_mul_overflow, "v.", "nt")
+BUILTIN(__builtin_add_overflow, "b.", "nt")
+BUILTIN(__builtin_sub_overflow, "b.", "nt")
+BUILTIN(__builtin_mul_overflow, "b.", "nt")
 BUILTIN(__builtin_uadd_overflow, "bUiCUiCUi*", "n")
 BUILTIN(__builtin_uaddl_overflow, "bULiCULiCULi*", "n")
 BUILTIN(__builtin_uaddll_overflow, "bULLiCULLiCULLi*", "n")


Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -1398,9 +1398,9 @@
 BUILTIN(__builtin_subcll, "ULLiULLiCULLiCULLiCULLi*", "n")
 
 // Checked Arithmetic Builtins for Security.
-BUILTIN(__builtin_add_overflow, "v.", "nt")
-BUILTIN(__builtin_sub_overflow, "v.", "nt")
-BUILTIN(__builtin_mul_overflow, "v.", "nt")
+BUILTIN(__builtin_add_overflow, "b.", "nt")
+BUILTIN(__builtin_sub_overflow, "b.", "nt")
+BUILTIN(__builtin_mul_overflow, "b.", "nt")
 BUILTIN(__builtin_uadd_overflow, "bUiCUiCUi*", "n")
 BUILTIN(__builtin_uaddl_overflow, "bULiCULiCULi*", "n")
 BUILTIN(__builtin_uaddll_overflow, "bULLiCULLiCULLi*", "n")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

ping


Repository:
  rC Clang

https://reviews.llvm.org/D52879



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

[PATCH] D53871: [OpenCL] Allow clk_event_t comparisons

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

LGTM, but please add a comment in the test file.




Comment at: test/SemaOpenCL/clk_event_t.cl:3
 
+#define CLK_NULL_EVENT (__builtin_astype(((void*)(__SIZE_MAX__)), clk_event_t))
+

It would be nice to have a comment saying this macro was extracted from the 
`opencl-c.h` header file.


Repository:
  rC Clang

https://reviews.llvm.org/D53871



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

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347658: Derive builtin return type from its definition 
(authored by mantognini, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D52879

Files:
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGenOpenCL/builtins.cl
  test/CodeGenOpenCL/pipe_builtin.cl

Index: test/CodeGenOpenCL/builtins.cl
===
--- test/CodeGenOpenCL/builtins.cl
+++ test/CodeGenOpenCL/builtins.cl
@@ -0,0 +1,83 @@
+// RUN: %clang_cc1 %s -finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
+
+void testBranchingOnEnqueueKernel(queue_t default_queue, unsigned flags, ndrange_t ndrange) {
+// Ensure `enqueue_kernel` can be branched upon.
+
+if (enqueue_kernel(default_queue, flags, ndrange, ^(void) {}))
+(void)0;
+// CHECK: [[P:%[0-9]+]] = call i32 @__enqueue_kernel
+// CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+// CHECK-NEXT: br i1 [[Q]]
+
+if (get_kernel_work_group_size(^(void) {}))
+(void)0;
+// CHECK: [[P:%[0-9]+]] = call i32 @__get_kernel_work_group_size
+// CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+// CHECK-NEXT: br i1 [[Q]]
+
+if (get_kernel_preferred_work_group_size_multiple(^(void) {}))
+(void)0;
+// CHECK: [[P:%[0-9]+]] = call i32 @__get_kernel_preferred_work_group_size_multiple_impl
+// CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+// CHECK-NEXT: br i1 [[Q]]
+}
+
+void testBranchinOnPipeOperations(read_only pipe int r, write_only pipe int w, global int* ptr) {
+// Verify that return type is correctly casted to i1 value.
+
+if (read_pipe(r, ptr))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__read_pipe_2
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (write_pipe(w, ptr))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__write_pipe_2
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (get_pipe_num_packets(r))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_num_packets_ro
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (get_pipe_num_packets(w))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_num_packets_wo
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (get_pipe_max_packets(r))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_max_packets_ro
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (get_pipe_max_packets(w))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_max_packets_wo
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+}
+
+void testBranchingOnAddressSpaceCast(generic long* ptr) {
+// Verify that pointer types are properly casted, respecting address spaces.
+
+if (to_global(ptr))
+(void)0;
+// CHECK:   [[P:%[0-9]+]] = call [[GLOBAL_VOID:i8 addrspace\(1\)\*]] @__to_global([[GENERIC_VOID:i8 addrspace\(4\)\*]] {{%[0-9]+}})
+// CHECK-NEXT:  [[Q:%[0-9]+]] = bitcast [[GLOBAL_VOID]] [[P]] to [[GLOBAL_i64:i64 addrspace\(1\)\*]]
+// CHECK-NEXT:  [[BOOL:%[a-z0-9]+]] = icmp ne [[GLOBAL_i64]] [[Q]], null
+// CHECK-NEXT:  br i1 [[BOOL]]
+
+if (to_local(ptr))
+(void)0;
+// CHECK:   [[P:%[0-9]+]] = call [[LOCAL_VOID:i8 addrspace\(3\)\*]] @__to_local([[GENERIC_VOID]] {{%[0-9]+}})
+// CHECK-NEXT:  [[Q:%[0-9]+]] = bitcast [[LOCAL_VOID]] [[P]] to [[LOCAL_i64:i64 addrspace\(3\)\*]]
+// CHECK-NEXT:  [[BOOL:%[a-z0-9]+]] = icmp ne [[LOCAL_i64]] [[Q]], null
+// CHECK-NEXT:  br i1 [[BOOL]]
+
+if (to_private(ptr))
+(void)0;
+// CHECK:   [[P:%[0-9]+]] = call [[PRIVATE_VOID:i8\*]] @__to_private([[GENERIC_VOID]] {{%[0-9]+}})
+// CHECK-NEXT:  [[Q:%[0-9]+]] = bitcast [[PRIVATE_VOID]] [[P]] to [[PRIVATE_i64:i64\*]]
+// CHECK-NEXT:  [[BOOL:%[a-z0-9]+]] = icmp ne [[PRIVATE_i64]] [[Q]], null
+// CHECK-NEXT:  br i1 [[BOOL]]
+}
+
Index: test/CodeGenOpenCL/pipe_builtin.cl
===
--- test/CodeGenOpenCL/pipe_builtin.cl
+++ test/CodeGenOpenCL/pipe_builtin.cl
@@ -69,25 +69,3 @@
   // CHECK: call i32 @__get_pipe_max_packets_wo(%opencl.pipe_wo_t* %{{.*}}, i32 4, i32 4)
   *ptr = get_pipe_max_packets(p);
 }
-
-void test9(read_only pipe int r, write_only pipe int w, global int *ptr) {
-  // verify that return type is correctly casted to i1 value
-  // CHECK: %[[R:[0-9]+]] = call i32 @__read_pipe_2
-  // CHECK: icmp ne i32 %[[R]], 0
-  if (read_pipe(r, ptr)) *ptr = -1;
-  // CHECK: %[[W:[0-9]+]] = call i32 @__write_pipe_2
-  // CHECK: icmp ne i32 %[[W]], 0
-  if (write_pipe(w, ptr)) *ptr = -1;
-  // CHECK: %[[NR:[0-9]+]] = call i32 @__get_pipe_num_packets_ro
-  // CHECK: icmp ne i32 %[[NR]], 0
-  if (get_pipe_num_packets(r)) *ptr = -1;
-  // CHECK: %[[NW:[0-9]+]] = call i32 @__get_pipe_num_packet

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D52879#1309734 , @riccibruno wrote:

> I see plenty of `TheCall->setType` left in `Sema::CheckBuiltinFunctionCall`
>  (`Builtin::BI__builtin_classify_type`, `Builtin::BI__builtin_constant_p`,
>  `Builtin::BI__builtin_dump_struct` and so on...).
>
> Is there a reason for not removing them ?


Mainly because I was focusing on OpenCL builtins and was confident about 
changing those, but wanted to be conservative on code that I don't test/work 
with. If one wanted to remove those, I would encourage them to double check 
their definitions are correct as I had to fix (https://reviews.llvm.org/D52875) 
some for OpenCL.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879



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

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D52879#1311177 , @riccibruno wrote:

> And moreover I believe this change is subtly incorrect for the following 
> reason:
>  The type that is passed into the constructor of the call expression is the 
> type
>  of the call expression, and not the return type.
>
> The difference between the return type and the type of the call expression is 
> that
>  1.) References are dropped, and
>  2.) For C++: The cv-qualifiers of non class pr-values are dropped,
>  3.) For C: The cv-qualifiers are dropped.
>
> ( as can be seen in `FunctionType::getCallResultType` )


Could you clarify one thing for me: If I understood what you were saying, the 
type passed to CallExpr should not be ref-cv-qualified, is that right?

In that case, couldn't we "simply" remove the ref + CV-qualifiers from 
ReturnTy? (`getNonReferenceType().withoutLocalFastQualifiers()`)

Note: I cannot revert this //and// keep the test because the new ones will fail.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879



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

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Thank you for the detailed review. I'll work on a patch and add you as reviewer 
once done (prob. on Monday though).


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879



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

[PATCH] D55136: [OpenCL][Sema] Improve BuildResolvedCallExpr handling of builtins

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
mantognini added a reviewer: riccibruno.
Herald added subscribers: cfe-commits, kristina, yaxunl.

This is a follow-up on https://reviews.llvm.org/D52879, addressing a few issues.

This:

- adds a FIXME for later improvement for specific builtins: I previously have 
only checked OpenCL ones and ensured tests cover those.
- fixed the CallExpr type.


Repository:
  rC Clang

https://reviews.llvm.org/D55136

Files:
  lib/Sema/SemaExpr.cpp


Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5549,17 +5549,19 @@
   // We special-case function promotion here because we only allow promoting
   // builtin functions to function pointers in the callee of a call.
   ExprResult Result;
-  QualType ReturnTy;
+  QualType ResultTy;
   if (BuiltinID &&
   Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
 // Extract the return type from the (builtin) function pointer type.
-auto FnPtrTy = Context.getPointerType(FDecl->getType());
+// FIXME Several builtins still have setType in 
Sema::CheckBuiltinFunctionCall.
+//   One should review their definitions in Builtins.def to ensure they
+//   are correct before removing setType calls.
+QualType FnPtrTy = Context.getPointerType(FDecl->getType());
 Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
-auto FnTy = FnPtrTy->getPointeeType()->castAs();
-ReturnTy = FnTy->getReturnType();
+ResultTy = FDecl->getCallResultType();
   } else {
 Result = CallExprUnaryConversions(Fn);
-ReturnTy = Context.BoolTy;
+ResultTy = Context.BoolTy;
   }
   if (Result.isInvalid())
 return ExprError();
@@ -5571,10 +5573,10 @@
   if (Config)
 TheCall =
 new (Context) CUDAKernelCallExpr(Context, Fn, cast(Config),
- Args, ReturnTy, VK_RValue, RParenLoc);
+ Args, ResultTy, VK_RValue, RParenLoc);
   else
 TheCall = new (Context)
-CallExpr(Context, Fn, Args, ReturnTy, VK_RValue, RParenLoc);
+CallExpr(Context, Fn, Args, ResultTy, VK_RValue, RParenLoc);
 
   if (!getLangOpts().CPlusPlus) {
 // C cannot always handle TypoExpr nodes in builtin calls and direct


Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5549,17 +5549,19 @@
   // We special-case function promotion here because we only allow promoting
   // builtin functions to function pointers in the callee of a call.
   ExprResult Result;
-  QualType ReturnTy;
+  QualType ResultTy;
   if (BuiltinID &&
   Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
 // Extract the return type from the (builtin) function pointer type.
-auto FnPtrTy = Context.getPointerType(FDecl->getType());
+// FIXME Several builtins still have setType in Sema::CheckBuiltinFunctionCall.
+//   One should review their definitions in Builtins.def to ensure they
+//   are correct before removing setType calls.
+QualType FnPtrTy = Context.getPointerType(FDecl->getType());
 Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
-auto FnTy = FnPtrTy->getPointeeType()->castAs();
-ReturnTy = FnTy->getReturnType();
+ResultTy = FDecl->getCallResultType();
   } else {
 Result = CallExprUnaryConversions(Fn);
-ReturnTy = Context.BoolTy;
+ResultTy = Context.BoolTy;
   }
   if (Result.isInvalid())
 return ExprError();
@@ -5571,10 +5573,10 @@
   if (Config)
 TheCall =
 new (Context) CUDAKernelCallExpr(Context, Fn, cast(Config),
- Args, ReturnTy, VK_RValue, RParenLoc);
+ Args, ResultTy, VK_RValue, RParenLoc);
   else
 TheCall = new (Context)
-CallExpr(Context, Fn, Args, ReturnTy, VK_RValue, RParenLoc);
+CallExpr(Context, Fn, Args, ResultTy, VK_RValue, RParenLoc);
 
   if (!getLangOpts().CPlusPlus) {
 // C cannot always handle TypoExpr nodes in builtin calls and direct
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Please see https://reviews.llvm.org/D55136 for the patch addressing these 
issues.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879



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

[PATCH] D55136: [OpenCL][Sema] Improve BuildResolvedCallExpr handling of builtins

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348120: [OpenCL][Sema] Improve BuildResolvedCallExpr 
handling of builtins (authored by mantognini, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55136?vs=176157&id=176353#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55136

Files:
  cfe/trunk/lib/Sema/SemaExpr.cpp


Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -5562,17 +5562,20 @@
   // We special-case function promotion here because we only allow promoting
   // builtin functions to function pointers in the callee of a call.
   ExprResult Result;
-  QualType ReturnTy;
+  QualType ResultTy;
   if (BuiltinID &&
   Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
 // Extract the return type from the (builtin) function pointer type.
-auto FnPtrTy = Context.getPointerType(FDecl->getType());
+// FIXME Several builtins still have setType in
+//   Sema::CheckBuiltinFunctionCall. One should review their
+//   definitions in Builtins.def to ensure they are correct before
+//   removing setType calls.
+QualType FnPtrTy = Context.getPointerType(FDecl->getType());
 Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
-auto FnTy = FnPtrTy->getPointeeType()->castAs();
-ReturnTy = FnTy->getReturnType();
+ResultTy = FDecl->getCallResultType();
   } else {
 Result = CallExprUnaryConversions(Fn);
-ReturnTy = Context.BoolTy;
+ResultTy = Context.BoolTy;
   }
   if (Result.isInvalid())
 return ExprError();
@@ -5584,10 +5587,10 @@
   if (Config)
 TheCall =
 new (Context) CUDAKernelCallExpr(Context, Fn, cast(Config),
- Args, ReturnTy, VK_RValue, RParenLoc);
+ Args, ResultTy, VK_RValue, RParenLoc);
   else
 TheCall = new (Context)
-CallExpr(Context, Fn, Args, ReturnTy, VK_RValue, RParenLoc);
+CallExpr(Context, Fn, Args, ResultTy, VK_RValue, RParenLoc);
 
   if (!getLangOpts().CPlusPlus) {
 // C cannot always handle TypoExpr nodes in builtin calls and direct


Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -5562,17 +5562,20 @@
   // We special-case function promotion here because we only allow promoting
   // builtin functions to function pointers in the callee of a call.
   ExprResult Result;
-  QualType ReturnTy;
+  QualType ResultTy;
   if (BuiltinID &&
   Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
 // Extract the return type from the (builtin) function pointer type.
-auto FnPtrTy = Context.getPointerType(FDecl->getType());
+// FIXME Several builtins still have setType in
+//   Sema::CheckBuiltinFunctionCall. One should review their
+//   definitions in Builtins.def to ensure they are correct before
+//   removing setType calls.
+QualType FnPtrTy = Context.getPointerType(FDecl->getType());
 Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
-auto FnTy = FnPtrTy->getPointeeType()->castAs();
-ReturnTy = FnTy->getReturnType();
+ResultTy = FDecl->getCallResultType();
   } else {
 Result = CallExprUnaryConversions(Fn);
-ReturnTy = Context.BoolTy;
+ResultTy = Context.BoolTy;
   }
   if (Result.isInvalid())
 return ExprError();
@@ -5584,10 +5587,10 @@
   if (Config)
 TheCall =
 new (Context) CUDAKernelCallExpr(Context, Fn, cast(Config),
- Args, ReturnTy, VK_RValue, RParenLoc);
+ Args, ResultTy, VK_RValue, RParenLoc);
   else
 TheCall = new (Context)
-CallExpr(Context, Fn, Args, ReturnTy, VK_RValue, RParenLoc);
+CallExpr(Context, Fn, Args, ResultTy, VK_RValue, RParenLoc);
 
   if (!getLangOpts().CPlusPlus) {
 // C cannot always handle TypoExpr nodes in builtin calls and direct
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52873: Remove unwanted signedness conversion from tests

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
mantognini added a reviewer: Anastasia.
Herald added a subscriber: cfe-commits.

This is just a minor update of the tests.


Repository:
  rC Clang

https://reviews.llvm.org/D52873

Files:
  test/SemaOpenCL/cl20-device-side-enqueue.cl


Index: test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -212,15 +212,15 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : enable
 
-kernel void foo(global int *buf)
+kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // 
expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', 
expected 'ndrange_t' argument type}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // 
expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', 
expected block argument type}}
 }
 
-kernel void bar(global int *buf)
+kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
@@ -230,13 +230,13 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : disable
 
-kernel void foo1(global int *buf)
+kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // 
expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' 
requires cl_khr_subgroups extension to be enabled}}
 }
 
-kernel void bar1(global int *buf)
+kernel void bar1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-error 
{{use of declaration 'get_kernel_sub_group_count_for_ndrange' requires 
cl_khr_subgroups extension to be enabled}}


Index: test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -212,15 +212,15 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : enable
 
-kernel void foo(global int *buf)
+kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected block argument type}}
 }
 
-kernel void bar(global int *buf)
+kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
@@ -230,13 +230,13 @@
 
 #pragma OPENCL EXTENSION cl_khr_subgroups : disable
 
-kernel void foo1(global int *buf)
+kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' requires cl_khr_subgroups extension to be enabled}}
 }
 
-kernel void bar1(global int *buf)
+kernel void bar1(global unsigned int *buf)
 {
   ndrange_t n;
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_sub_group_count_for_ndrange' requires cl_khr_subgroups extension to be enabled}}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52875: Fix definitions of __builtin_(add|sub|mul)_overflow

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
mantognini added reviewers: Anastasia, erichkeane.
Herald added subscribers: cfe-commits, kristina.

Ensure __builtin_(add|sub|mul)_overflow return bool instead of void as per 
specification (LanguageExtensions).


Repository:
  rC Clang

https://reviews.llvm.org/D52875

Files:
  include/clang/Basic/Builtins.def


Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -1397,9 +1397,9 @@
 BUILTIN(__builtin_subcll, "ULLiULLiCULLiCULLiCULLi*", "n")
 
 // Checked Arithmetic Builtins for Security.
-BUILTIN(__builtin_add_overflow, "v.", "nt")
-BUILTIN(__builtin_sub_overflow, "v.", "nt")
-BUILTIN(__builtin_mul_overflow, "v.", "nt")
+BUILTIN(__builtin_add_overflow, "b.", "nt")
+BUILTIN(__builtin_sub_overflow, "b.", "nt")
+BUILTIN(__builtin_mul_overflow, "b.", "nt")
 BUILTIN(__builtin_uadd_overflow, "bUiCUiCUi*", "n")
 BUILTIN(__builtin_uaddl_overflow, "bULiCULiCULi*", "n")
 BUILTIN(__builtin_uaddll_overflow, "bULLiCULLiCULLi*", "n")


Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -1397,9 +1397,9 @@
 BUILTIN(__builtin_subcll, "ULLiULLiCULLiCULLiCULLi*", "n")
 
 // Checked Arithmetic Builtins for Security.
-BUILTIN(__builtin_add_overflow, "v.", "nt")
-BUILTIN(__builtin_sub_overflow, "v.", "nt")
-BUILTIN(__builtin_mul_overflow, "v.", "nt")
+BUILTIN(__builtin_add_overflow, "b.", "nt")
+BUILTIN(__builtin_sub_overflow, "b.", "nt")
+BUILTIN(__builtin_mul_overflow, "b.", "nt")
 BUILTIN(__builtin_uadd_overflow, "bUiCUiCUi*", "n")
 BUILTIN(__builtin_uaddl_overflow, "bULiCULiCULi*", "n")
 BUILTIN(__builtin_uaddll_overflow, "bULLiCULLiCULLi*", "n")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
mantognini added reviewers: Anastasia, spatel.
Herald added subscribers: cfe-commits, kristina.
mantognini added dependencies: D52873: Remove unwanted signedness conversion 
from tests, D52875: Fix definitions of __builtin_(add|sub|mul)_overflow.

Prior to this patch, OpenCL code such as the following would attempt to create
a BranchInst with a non-bool argument:

  if (enqueue_kernel(get_default_queue(), 0, nd, ^(void){})) /* ... */

This patch is a follow up on a similar issue with pipe builtin
operations. See commit r280800 and https://bugs.llvm.org/show_bug.cgi?id=30219.

This change, while being conservative on non-builtin functions,
should set the type of expressions invoking builtins to the
proper type, instead of defaulting to `bool` and requiring
manual overrides in Sema::CheckBuiltinFunctionCall.

In addition to tests for enqueue_kernel, the tests are extended to
check other OpenCL builtins.


Repository:
  rC Clang

https://reviews.llvm.org/D52879

Files:
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGenOpenCL/builtins.cl
  test/CodeGenOpenCL/pipe_builtin.cl

Index: test/CodeGenOpenCL/pipe_builtin.cl
===
--- test/CodeGenOpenCL/pipe_builtin.cl
+++ test/CodeGenOpenCL/pipe_builtin.cl
@@ -69,25 +69,3 @@
   // CHECK: call i32 @__get_pipe_max_packets_wo(%opencl.pipe_wo_t* %{{.*}}, i32 4, i32 4)
   *ptr = get_pipe_max_packets(p);
 }
-
-void test9(read_only pipe int r, write_only pipe int w, global int *ptr) {
-  // verify that return type is correctly casted to i1 value
-  // CHECK: %[[R:[0-9]+]] = call i32 @__read_pipe_2
-  // CHECK: icmp ne i32 %[[R]], 0
-  if (read_pipe(r, ptr)) *ptr = -1;
-  // CHECK: %[[W:[0-9]+]] = call i32 @__write_pipe_2
-  // CHECK: icmp ne i32 %[[W]], 0
-  if (write_pipe(w, ptr)) *ptr = -1;
-  // CHECK: %[[NR:[0-9]+]] = call i32 @__get_pipe_num_packets_ro
-  // CHECK: icmp ne i32 %[[NR]], 0
-  if (get_pipe_num_packets(r)) *ptr = -1;
-  // CHECK: %[[NW:[0-9]+]] = call i32 @__get_pipe_num_packets_wo
-  // CHECK: icmp ne i32 %[[NW]], 0
-  if (get_pipe_num_packets(w)) *ptr = -1;
-  // CHECK: %[[MR:[0-9]+]] = call i32 @__get_pipe_max_packets_ro
-  // CHECK: icmp ne i32 %[[MR]], 0
-  if (get_pipe_max_packets(r)) *ptr = -1;
-  // CHECK: %[[MW:[0-9]+]] = call i32 @__get_pipe_max_packets_wo
-  // CHECK: icmp ne i32 %[[MW]], 0
-  if (get_pipe_max_packets(w)) *ptr = -1;
-}
Index: test/CodeGenOpenCL/builtins.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/builtins.cl
@@ -0,0 +1,83 @@
+// RUN: %clang_cc1 %s -finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
+
+void testBranchingOnEnqueueKernel(queue_t default_queue, unsigned flags, ndrange_t ndrange) {
+// Ensure `enqueue_kernel` can be branched upon.
+
+if (enqueue_kernel(default_queue, flags, ndrange, ^(void) {}))
+(void)0;
+// CHECK: [[P:%[0-9]+]] = call i32 @__enqueue_kernel
+// CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+// CHECK-NEXT: br i1 [[Q]]
+
+if (get_kernel_work_group_size(^(void) {}))
+(void)0;
+// CHECK: [[P:%[0-9]+]] = call i32 @__get_kernel_work_group_size
+// CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+// CHECK-NEXT: br i1 [[Q]]
+
+if (get_kernel_preferred_work_group_size_multiple(^(void) {}))
+(void)0;
+// CHECK: [[P:%[0-9]+]] = call i32 @__get_kernel_preferred_work_group_size_multiple_impl
+// CHECK-NEXT: [[Q:%[a-z0-9]+]] = icmp ne i32 [[P]], 0
+// CHECK-NEXT: br i1 [[Q]]
+}
+
+void testBranchinOnPipeOperations(read_only pipe int r, write_only pipe int w, global int* ptr) {
+// Verify that return type is correctly casted to i1 value.
+
+if (read_pipe(r, ptr))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__read_pipe_2
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (write_pipe(w, ptr))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__write_pipe_2
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (get_pipe_num_packets(r))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_num_packets_ro
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (get_pipe_num_packets(w))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_num_packets_wo
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (get_pipe_max_packets(r))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_max_packets_ro
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+
+if (get_pipe_max_packets(w))
+(void)0;
+// CHECK: [[R:%[0-9]+]] = call i32 @__get_pipe_max_packets_wo
+// CHECK-NEXT: icmp ne i32 [[R]], 0
+}
+
+void testBranchingOnAddressSpaceCast(generic long* ptr) {
+// Verify that pointer types are properly casted, respecting address spaces.
+
+if (to_global(ptr))
+(void)0;
+// CHECK:   [[P:%[0-9]+]] = call [[GLOBAL_VOID:i8

[PATCH] D52875: Fix definitions of __builtin_(add|sub|mul)_overflow

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In https://reviews.llvm.org/D52875#1255146, @erichkeane wrote:

> Can you write tests for this please?  Particularly validate the results in a 
> constexpr context.


There are already some tests for those builtins (not sure about constexpr 
context). They already tests that the builtins can be used as branching 
condition. However, the current implementation of `Sema::BuildResolvedCallExpr` 
assumes that by default builtins return `bool`. In 
https://reviews.llvm.org/D52879, I improve that and not having the above fix 
makes the existing tests fail, so I believe we don't need to add more tests.

> Additionally, these all have the 't' flag, which means that these signatures 
> are meaningless, right?  What are you seeing where this works incorrectly?

I reckon the signature does't include the return type, hence it isn't 
meaningless even with the `t` flag.


Repository:
  rC Clang

https://reviews.llvm.org/D52875



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

[PATCH] D52873: Remove unwanted signedness conversion from tests

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Note: the `get_kernel_*` functions used here all return unsigned integers, 
hence the warning/present patch.


Repository:
  rC Clang

https://reviews.llvm.org/D52873



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

[PATCH] D62591: [OpenCL][PR42031] Prevent deducing addr space in type alias.

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D62591



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

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

[Marco Antognini via Phabricator via cfe-commits]

mantognini marked 2 inline comments as done.
mantognini added inline comments.



Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229
+LangAS AddrSpaceR =
+RHSType->getAs()->getPointeeType().getAddressSpace();
+CastKind Kind =

Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Anastasia wrote:
> > > > > rjmccall wrote:
> > > > > > All of this can be much simpler:
> > > > > > 
> > > > > > ```
> > > > > > LangAS AddrSpaceL = 
> > > > > > ToType->castAs()->getPointeeType().getAddressSpace();
> > > > > > LangAS AddrSpaceR = 
> > > > > > FromType->castAs()->getPointeeType().getAddressSpace();
> > > > > > ```
> > > > > > 
> > > > > > Is there something actually checking the validity of this 
> > > > > > address-space cast somewhere?
> > > > > > Is there something actually checking the validity of this 
> > > > > > address-space cast somewhere?
> > > > > 
> > > > > The address spaces for blocks are currently added by clang 
> > > > > implicitly. It doesn't seem possible to write kernel code qualifying 
> > > > > blocks with address spaces. Although I have to say the error is not 
> > > > > given properly because the parser gets confused at least for the 
> > > > > examples I have tried. The OpenCL spec doesn't detail much regarding 
> > > > > this use case. Potentially this is the area for improvement.
> > > > > 
> > > > > So for now we can add an assert to check the cast validity if you 
> > > > > think it makes sense and maybe a FIXME in the  code to explain that 
> > > > > address spaces aren't working with blocks
> > > > > The address spaces for blocks are currently added by clang 
> > > > > implicitly. It doesn't seem possible to write kernel code qualifying 
> > > > > blocks with address spaces.
> > > > 
> > > > There's no way that just fell out from the existing implementation; it 
> > > > was a change someone must have made for OpenCL.  Unless you care about 
> > > > blocks existing in multiple address spaces — which, given that you 
> > > > depend on eliminating them, I'm pretty sure you don't — the compiler 
> > > > just shouldn't do that if it's causing you problems.
> > > So the reasons why we add addr spaces to blocks is that if they are 
> > > declared in program scope they will be inferred as `__global` AS and if 
> > > they are declared in kernel scope they are inferred as `__private` AS.
> > > 
> > > When there is a common code i.e. we pass block into a function or invoke 
> > > the block we use generic AS so that blocks in different addr spaces can 
> > > be work correctly but we are adding addr space cast.
> > > 
> > > This is the review that added this logic for OpenCL C: 
> > > https://reviews.llvm.org/D28814
> > > 
> > > However in C++ we follow slightly different program path and therefore 
> > > addr space cast isn't performed correctly.
> > Okay, so users can't write block pointer types with explicit address 
> > spaces.  What exactly do you mean by "declaring" a block?  Do you mean that 
> > block pointer *types* are inferred to have different qualification based on 
> > where they're written, or do you mean that block *literals* have different 
> > qualification based on where they're written?  Because I'm not sure the 
> > latter is actually distinguishable from a language model in which blocks 
> > are always pointers into `__generic` and the compiler just immediately 
> > promotes them when emitting the expression.
> We add `__generic` addr space to pointee type of block pointer type for all 
> block variables. However, we don't do the same for block literals. Hence we 
> need to generate conversion from `LangAS::Default` to 
> `LangAS::opencl_generic`... I think this just aligns with what we do for 
> other similar cases in OpenCL.
> 
> We also add `__global`/`__private` to block pointer type in block variable 
> declarations, but we do the same for all other objects. At IR generation we 
> generate block literals with captures as local variables in `__private` addr 
> space and block literals without captures as global variables in `__global` 
> addr space.
I've been experimenting a bit more with blocks. Here is a snippet that helped 
me understand things further:

```
typedef int (^block_ty)(void);
__global block_ty block3 = ^{ return 0; };
__global int (^block4)(void) = ^{ return 0; }; // FIXME return type is not 
allowed to have AS qualifier

kernel void test(void) {
  block_ty __constant block0 = ^{ return 0; };
  int (^block1)(void) = ^(void){ return 0; };
  __private block_ty block2 = ^{ return 0; };
  // block2 = ^{ return 1; }; // invalid code in OpenCL; blocks cannot be 
re-assigned.

  int x = ((__local block_ty) block3)(); // FIXME The AS cast is missing from 
the AST, plus Clang crashes.
}
```

This piece of code shows two bugs:
* Address space qualifier on the return type of blocks should be rejected.
* Address space cast are missing from the AST when doing a C cast, and this 
regar

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 210080.
mantognini added a comment.

- Refactored common bits from CheckConstructorDeclarator and 
CheckDestructorDeclarator.
- Added as many "ThisTy" parameter I could.
- Addressed issue with identifier format in tests (%N to %var.ascast).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569

Files:
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
  clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl

Index: clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS
+
+// This test ensures the proper address spaces and address space cast are used
+// for constructors, member functions and destructors.
+// See also atexit.cl and global_init.cl for other specific tests.
+
+// CHECK: %struct.MyType = type { i32 }
+struct MyType {
+  MyType(int i) : i(i) {}
+  MyType(int i) __constant : i(i) {}
+  ~MyType() {}
+  ~MyType() __constant {}
+  int bar() { return i + 2; }
+  int bar() __constant { return i + 1; }
+  int i;
+};
+
+// CHECK: @const1 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const1 = 1;
+// CHECK: @const2 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const2(2);
+// CHECK: @glob = addrspace(1) global %struct.MyType zeroinitializer
+MyType glob(1);
+
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
+// CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*), i32 1)
+
+// CHECK-LABEL: define spir_kernel void @fooGlobal()
+kernel void fooGlobal() {
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*))
+  glob.bar();
+  // CHECK: call i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* @const1)
+  const1.bar();
+  // CHECK: call void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* @const1)
+  const1.~MyType();
+}
+
+// CHECK-LABEL: define spir_kernel void @fooLocal()
+kernel void fooLocal() {
+  // CHECK: [[VAR:%.*]] = alloca %struct.MyType
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* [[REG]], i32 3)
+  MyType myLocal(3);
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* [[REG]])
+  myLocal.bar();
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* [[REG]])
+}
+
+// Ensure all members are defined for all the required address spaces.
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* %this)
Index: clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
===
--- clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
-
-struct MyType {
-  MyType(int i) : i(i) {}
-  MyType(int i) __constant : i(i) {}
-  int i;
-};
-
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
-__constant MyType const1 = 1;
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
-__constant MyType const2(2);
-//CHECK: call void @_ZNU3A

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini marked 10 inline comments as done.
mantognini added a comment.

Mind the fact that I've rebased the changes onto a more recent master version. 
If you look at the diff of v1 against v2 you might see some unrelated changes.

Let me know if there's anything else that I need to change.




Comment at: clang/lib/CodeGen/CGClass.cpp:2016
   CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
-/*Delegating=*/false, addr);
+/*Delegating=*/false, addr, type);
 }

rjmccall wrote:
> mantognini wrote:
> > This is the only place where the new parameter has an interesting value. In 
> > the ~10 other calls to `EmitCXXDestructorCall` overloads, the default value 
> > of this new parameter is used instead.
> Arguments that are potentially required for correctness — as opposed to just 
> enabling some optimization — should generally not have defaults.  I think you 
> should remove these defaults and require a sensible type to be passed down in 
> all cases.
I've addressed that. I had to add some extra parameter/attributes here and 
there. Please let me know if anything is can be improved.



Comment at: clang/lib/CodeGen/CGClass.cpp:1447
+if (HaveInsertPoint()) {
+  // FIXME: Determine the type of the object being destroyed.
+  QualType ThisTy;

I'm not familiar enough with CXXCtorType and such, and would welcome some help 
for these two FIXMEs.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:103
+  // we ensure a cast is added where necessary.
+  if (ThisTy.isNull()) {
+#ifndef NDEBUG

Despite no longer having a default parameter, not all call site can provide a 
meaningful value ATM. That is why this check is still required.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:96
+llvm::Value *ImplicitParam, QualType ImplicitParamTy, const CallExpr *CE,
+QualType ThisTy) {
+  const CXXMethodDecl *DtorDecl = cast(Dtor.getDecl());

mantognini wrote:
> This new parameter is required in order to call `performAddrSpaceCast`.
Now that it's no longer a default parameter. I've group ThisTy with This.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:117-118
+  llvm::Type *NewType = CGM.getTypes().ConvertType(DstTy);
+  This = getTargetHooks().performAddrSpaceCast(*this, This, SrcAS, DstAS,
+   NewType);
+}

rjmccall wrote:
> Anastasia wrote:
> > mantognini wrote:
> > > This is effectively the fix for the third point mentioned in the 
> > > description.
> > > 
> > > Alternatively, `This = Builder.CreatePointerBitCastOrAddrSpaceCast(This, 
> > > NewType);` seems to work equally well and does not require the extra new 
> > > parameter.
> > Yes, I agree just using `CreatePointerBitCastOrAddrSpaceCast` would be 
> > easier.
> > 
> > As far as I remember (@rjmccall can correct me if I am wrong) 
> > `performAddrSpaceCast` was added to workaround the fact that `nullptr` 
> > constant might not be value 0 for some address spaces. However, considering 
> > that it's not the first place where some big change has to be done in 
> > CodeGen to be able to use the new API I am wondering if it's worth looking 
> > at moving this logic to LLVM lowering phases that can map `nullptr` 
> > constant into some arbitrary value. I think the challenge is to find where 
> > LLVM assumes that `nullptr` is always 0. That might not be an easy task.
> > 
> > PS, I am not suggesting to do it for this patch but just an idea to 
> > investigate in the future. 
> I continue to think that it makes sense to set Clang up to be able to handle 
> language-level address spaces that don't exist at the level of LLVM IR.

Alright, I've kept it.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8428-8439
+bool DiagOccured = false;
 FTI.MethodQualifiers->forEachQualifier(
 [&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) {
+  // This diagnostic should be emitted on any qualifier except an addr
+  // space qualifier. However, forEachQualifier currently doesn't visit
+  // addr space qualifiers, so there's no way to write this condition
+  // right now; we just diagnose on everything.

rjmccall wrote:
> mantognini wrote:
> > This is the fix for the first point in the description.
> Can we unify this with the similar check we do elsewhere?
Done.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8197
+  const DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
+  if (FTI.hasMethodTypeQualifiers() && !D.isInvalidType()) {
+bool DiagOccured = false;

I've refactored that bit out of the two mentioned functions. Mind the fact that 
it now always check `!D.isInvalidType()`. I think it makes sense, but let me 
know if that's not the case

[PATCH] D64804: [OpenCL][Sema] Minor refactoring and constraint checking

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: cfe-commits, Anastasia, yaxunl.
Herald added a project: clang.

Simplify code a bit and add assertion to address post-landing comments
from D64083 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64804

Files:
  clang/lib/Sema/SemaExprCXX.cpp


Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -4216,17 +4216,12 @@
 break;
 
   case ICK_Block_Pointer_Conversion: {
-QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
-QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
-
-// Assumptions based on Sema::IsBlockPointerConversion.
-assert(isa(LHSType) && "BlockPointerType expected");
-assert(isa(RHSType) && "BlockPointerType expected");
-
 LangAS AddrSpaceL =
-LHSType->getAs()->getPointeeType().getAddressSpace();
+ToType->castAs()->getPointeeType().getAddressSpace();
 LangAS AddrSpaceR =
-RHSType->getAs()->getPointeeType().getAddressSpace();
+
FromType->castAs()->getPointeeType().getAddressSpace();
+assert(Qualifiers::isAddressSpaceSupersetOf(AddrSpaceL, AddrSpaceR) &&
+   "Invalid cast");
 CastKind Kind =
 AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
 From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,


Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -4216,17 +4216,12 @@
 break;
 
   case ICK_Block_Pointer_Conversion: {
-QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
-QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
-
-// Assumptions based on Sema::IsBlockPointerConversion.
-assert(isa(LHSType) && "BlockPointerType expected");
-assert(isa(RHSType) && "BlockPointerType expected");
-
 LangAS AddrSpaceL =
-LHSType->getAs()->getPointeeType().getAddressSpace();
+ToType->castAs()->getPointeeType().getAddressSpace();
 LangAS AddrSpaceR =
-RHSType->getAs()->getPointeeType().getAddressSpace();
+FromType->castAs()->getPointeeType().getAddressSpace();
+assert(Qualifiers::isAddressSpaceSupersetOf(AddrSpaceL, AddrSpaceR) &&
+   "Invalid cast");
 CastKind Kind =
 AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
 From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64804: [OpenCL][Sema] Minor refactoring and constraint checking

[Marco Antognini via Phabricator via cfe-commits]

mantognini added reviewers: Anastasia, rjmccall.
mantognini added a comment.

This should address the minor refactoring requesting in D64083 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64804



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

[PATCH] D64804: [OpenCL][Sema] Minor refactoring and constraint checking

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366306: [OpenCL][Sema] Minor refactoring and constraint 
checking (authored by mantognini, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64804?vs=210115&id=210269#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64804

Files:
  cfe/trunk/lib/Sema/SemaExprCXX.cpp


Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -4216,17 +4216,12 @@
 break;
 
   case ICK_Block_Pointer_Conversion: {
-QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
-QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
-
-// Assumptions based on Sema::IsBlockPointerConversion.
-assert(isa(LHSType) && "BlockPointerType expected");
-assert(isa(RHSType) && "BlockPointerType expected");
-
 LangAS AddrSpaceL =
-LHSType->getAs()->getPointeeType().getAddressSpace();
+ToType->castAs()->getPointeeType().getAddressSpace();
 LangAS AddrSpaceR =
-RHSType->getAs()->getPointeeType().getAddressSpace();
+
FromType->castAs()->getPointeeType().getAddressSpace();
+assert(Qualifiers::isAddressSpaceSupersetOf(AddrSpaceL, AddrSpaceR) &&
+   "Invalid cast");
 CastKind Kind =
 AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
 From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,


Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -4216,17 +4216,12 @@
 break;
 
   case ICK_Block_Pointer_Conversion: {
-QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
-QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
-
-// Assumptions based on Sema::IsBlockPointerConversion.
-assert(isa(LHSType) && "BlockPointerType expected");
-assert(isa(RHSType) && "BlockPointerType expected");
-
 LangAS AddrSpaceL =
-LHSType->getAs()->getPointeeType().getAddressSpace();
+ToType->castAs()->getPointeeType().getAddressSpace();
 LangAS AddrSpaceR =
-RHSType->getAs()->getPointeeType().getAddressSpace();
+FromType->castAs()->getPointeeType().getAddressSpace();
+assert(Qualifiers::isAddressSpaceSupersetOf(AddrSpaceL, AddrSpaceR) &&
+   "Invalid cast");
 CastKind Kind =
 AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
 From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64418: [Docs][OpenCL] Documentation of C++ for OpenCL mode

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

Beside my two comments, I think this looks good.




Comment at: docs/LanguageExtensions.rst:1614
+
+By default references will refer to ``__generic`` address space objects 
(except for
+dependent types that are not template specializations

` (` -> `, ` (there's no matching closing parenthesis.)



Comment at: docs/LanguageExtensions.rst:1652-1657
+ __kernel void bar() {
+   __local C c1; // will resolve to the first foo
+   C c2; // will resolve to the second foo
+   __constant C c3; // error due to mismatching address spaces - can't convert 
to
+// __local or __generic
+ }

`foo()` isn't actually called here. You probably meant to write this:


```
__local C c1;
C c2;
__constant C c3;

c1.foo(); // will resolve to the first foo
c2.foo(); // will resolve to the second foo
c3.foo(); // error due to mismatching address spaces - can't convert to __local 
or __generic
```


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

https://reviews.llvm.org/D64418



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

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 210341.
mantognini marked 16 inline comments as done.
mantognini added a comment.

- Addressed comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
  clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl

Index: clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS
+
+// This test ensures the proper address spaces and address space cast are used
+// for constructors, member functions and destructors.
+// See also atexit.cl and global_init.cl for other specific tests.
+
+// CHECK: %struct.MyType = type { i32 }
+struct MyType {
+  MyType(int i) : i(i) {}
+  MyType(int i) __constant : i(i) {}
+  ~MyType() {}
+  ~MyType() __constant {}
+  int bar() { return i + 2; }
+  int bar() __constant { return i + 1; }
+  int i;
+};
+
+// CHECK: @const1 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const1 = 1;
+// CHECK: @const2 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const2(2);
+// CHECK: @glob = addrspace(1) global %struct.MyType zeroinitializer
+MyType glob(1);
+
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
+// CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*), i32 1)
+
+// CHECK-LABEL: define spir_kernel void @fooGlobal()
+kernel void fooGlobal() {
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*))
+  glob.bar();
+  // CHECK: call i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* @const1)
+  const1.bar();
+  // CHECK: call void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* @const1)
+  const1.~MyType();
+}
+
+// CHECK-LABEL: define spir_kernel void @fooLocal()
+kernel void fooLocal() {
+  // CHECK: [[VAR:%.*]] = alloca %struct.MyType
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* [[REG]], i32 3)
+  MyType myLocal(3);
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* [[REG]])
+  myLocal.bar();
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* [[REG]])
+}
+
+// Ensure all members are defined for all the required address spaces.
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* %this)
Index: clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
===
--- clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
-
-struct MyType {
-  MyType(int i) : i(i) {}
-  MyType(int i) __constant : i(i) {}
-  int i;
-};
-
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
-__constant MyType const1 = 1;
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
-__constant MyType const2(2);
-//CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType 

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini marked an inline comment as done.
mantognini added inline comments.



Comment at: clang/lib/CodeGen/CGClass.cpp:496
+  // destroyed should have the expected type.
+  QualType ThisTy = D->getThisType();
   Address Addr =

rjmccall wrote:
> I think the rule we want is that the type passed here is the (qualified) 
> object type, but `getThisType()` will return a pointer type.  Consider adding 
> a `getThisObjectType()` method to `CXXMethodDecl` which does that computation 
> (and then make `getThisType()` just wrap that in a `PointerType`).
I've done the suggested refactoring, let me know if it needs adjustment.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:103
+  // we ensure a cast is added where necessary.
+  if (ThisTy.isNull()) {
+#ifndef NDEBUG

rjmccall wrote:
> mantognini wrote:
> > Despite no longer having a default parameter, not all call site can provide 
> > a meaningful value ATM. That is why this check is still required.
> Is that resolved with fixing the FIXME?
> 
> Please assert that `ThisTy->getAsCXXRecordDecl() == Dtor->getParent()` to 
> guard against that pointer/object mixup.
Yes, indeed. I've therefore simplified the code and added the requested 
assertion.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:374
   EmitCXXDestructorCall(GD, Callee, This.getPointer(),
+/*ThisTy=*/QualType(),
 /*ImplicitParam=*/nullptr,

rjmccall wrote:
> `ThisTy` here can be either `Base->getType()` or 
> `Base->getType()->getPointeeType()` depending on whether this is an arrow 
> access.
Thanks for mentioning it. It should be better now.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:99
+  assert(!ThisTy.isNull());
+  assert(!ThisTy->isPointerType() && "Unexpected pointer type");
+  assert(ThisTy->getAsCXXRecordDecl() == DtorDecl->getParent() &&

I wasn't 100% sure if this was covered by the next assertion.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1758
+  // This is suboptimal for correctness purposes. Instead, CE should probably
+  // always be defined.
+  QualType ThisTy;

rjmccall wrote:
> It looks like the only places that pass down a null CE are the two 
> implementations in `emitVirtualObjectDelete`, which both have a 
> `CXXDeleteExpr`.  You could just have this method take an 
> `llvm::PointerUnion` or something, and 
> then someone else could take advantage of that for better source locations on 
> the virtual call eventually.
Alright, makes sense. I've changed the function to take a `PointerUnion` and 
removed the FIXMEs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569



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

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 210501.
mantognini marked 4 inline comments as done.
mantognini added a comment.

- Minor refactoring of getThisObjectType
- Removed unnecessary assertion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
  clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl

Index: clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS
+
+// This test ensures the proper address spaces and address space cast are used
+// for constructors, member functions and destructors.
+// See also atexit.cl and global_init.cl for other specific tests.
+
+// CHECK: %struct.MyType = type { i32 }
+struct MyType {
+  MyType(int i) : i(i) {}
+  MyType(int i) __constant : i(i) {}
+  ~MyType() {}
+  ~MyType() __constant {}
+  int bar() { return i + 2; }
+  int bar() __constant { return i + 1; }
+  int i;
+};
+
+// CHECK: @const1 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const1 = 1;
+// CHECK: @const2 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const2(2);
+// CHECK: @glob = addrspace(1) global %struct.MyType zeroinitializer
+MyType glob(1);
+
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
+// CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*), i32 1)
+
+// CHECK-LABEL: define spir_kernel void @fooGlobal()
+kernel void fooGlobal() {
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*))
+  glob.bar();
+  // CHECK: call i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* @const1)
+  const1.bar();
+  // CHECK: call void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* @const1)
+  const1.~MyType();
+}
+
+// CHECK-LABEL: define spir_kernel void @fooLocal()
+kernel void fooLocal() {
+  // CHECK: [[VAR:%.*]] = alloca %struct.MyType
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* [[REG]], i32 3)
+  MyType myLocal(3);
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* [[REG]])
+  myLocal.bar();
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* [[REG]])
+}
+
+// Ensure all members are defined for all the required address spaces.
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* %this)
Index: clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
===
--- clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
-
-struct MyType {
-  MyType(int i) : i(i) {}
-  MyType(int i) __constant : i(i) {}
-  int i;
-};
-
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
-__constant MyType const1 = 1;
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
-__constant MyType const2(2);
-//CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:2267
   QualType ClassTy = C.getTypeDeclType(Decl);
-  ClassTy = C.getQualifiedType(ClassTy, FPT->getMethodQuals());
-  return C.getPointerType(ClassTy);
+  return C.getQualifiedType(ClassTy, FPT->getMethodQuals());
 }

rjmccall wrote:
> Thanks.  Can you extract this implementation out into a `static` function 
> (i.e. an internal linkage function in this file) that takes an `ASTContext&` 
> so that `getThisType` doesn't have to fetch the `ASTContext` twice?
Sure, no problem.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:99
+  assert(!ThisTy.isNull());
+  assert(!ThisTy->isPointerType() && "Unexpected pointer type");
+  assert(ThisTy->getAsCXXRecordDecl() == DtorDecl->getParent() &&

rjmccall wrote:
> mantognini wrote:
> > I wasn't 100% sure if this was covered by the next assertion.
> It is.
Alright, thanks for the clarification. I've removed it as it doesn't bring 
anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569



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

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366422: [OpenCL] Improve destructor support in C++ for 
OpenCL (authored by mantognini, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64569?vs=210501&id=210513#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64569

Files:
  cfe/trunk/include/clang/AST/DeclCXX.h
  cfe/trunk/lib/AST/DeclCXX.cpp
  cfe/trunk/lib/CodeGen/CGCXXABI.h
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/CodeGen/CGClass.cpp
  cfe/trunk/lib/CodeGen/CGDecl.cpp
  cfe/trunk/lib/CodeGen/CGExprCXX.cpp
  cfe/trunk/lib/CodeGen/CodeGenFunction.h
  cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
  cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
  cfe/trunk/lib/Sema/SemaDeclCXX.cpp
  cfe/trunk/lib/Sema/SemaOverload.cpp
  cfe/trunk/test/CodeGenOpenCLCXX/addrspace-ctor.cl
  cfe/trunk/test/CodeGenOpenCLCXX/addrspace-with-class.cl

Index: cfe/trunk/include/clang/AST/DeclCXX.h
===
--- cfe/trunk/include/clang/AST/DeclCXX.h
+++ cfe/trunk/include/clang/AST/DeclCXX.h
@@ -2232,20 +2232,20 @@
 
   overridden_method_range overridden_methods() const;
 
-  /// Returns the parent of this method declaration, which
+  /// Return the parent of this method declaration, which
   /// is the class in which this method is defined.
   const CXXRecordDecl *getParent() const {
 return cast(FunctionDecl::getParent());
   }
 
-  /// Returns the parent of this method declaration, which
+  /// Return the parent of this method declaration, which
   /// is the class in which this method is defined.
   CXXRecordDecl *getParent() {
 return const_cast(
  cast(FunctionDecl::getParent()));
   }
 
-  /// Returns the type of the \c this pointer.
+  /// Return the type of the \c this pointer.
   ///
   /// Should only be called for instance (i.e., non-static) methods. Note
   /// that for the call operator of a lambda closure type, this returns the
@@ -2253,9 +2253,17 @@
   /// 'this' type.
   QualType getThisType() const;
 
+  /// Return the type of the object pointed by \c this.
+  ///
+  /// See getThisType() for usage restriction.
+  QualType getThisObjectType() const;
+
   static QualType getThisType(const FunctionProtoType *FPT,
   const CXXRecordDecl *Decl);
 
+  static QualType getThisObjectType(const FunctionProtoType *FPT,
+const CXXRecordDecl *Decl);
+
   Qualifiers getMethodQualifiers() const {
 return getType()->getAs()->getMethodQuals();
   }
Index: cfe/trunk/test/CodeGenOpenCLCXX/addrspace-with-class.cl
===
--- cfe/trunk/test/CodeGenOpenCLCXX/addrspace-with-class.cl
+++ cfe/trunk/test/CodeGenOpenCLCXX/addrspace-with-class.cl
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS
+
+// This test ensures the proper address spaces and address space cast are used
+// for constructors, member functions and destructors.
+// See also atexit.cl and global_init.cl for other specific tests.
+
+// CHECK: %struct.MyType = type { i32 }
+struct MyType {
+  MyType(int i) : i(i) {}
+  MyType(int i) __constant : i(i) {}
+  ~MyType() {}
+  ~MyType() __constant {}
+  int bar() { return i + 2; }
+  int bar() __constant { return i + 1; }
+  int i;
+};
+
+// CHECK: @const1 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const1 = 1;
+// CHECK: @const2 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const2(2);
+// CHECK: @glob = addrspace(1) global %struct.MyType zeroinitializer
+MyType glob(1);
+
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
+// CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*), i32 1)
+
+// CHECK-LABEL: define spir_kernel void @fooGlobal()
+kernel void fooGlobal() {
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*))
+  glob.bar();
+  // CHECK: call i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* @const1)
+  const1.bar();
+  // CHECK: call void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* @const1)
+  const1.~MyType();
+}
+
+// CHECK-LABEL: define spir_kernel void @fooLocal()
+kernel void fooLocal() {
+  // CHECK: [[VAR:%.*]] = alloca %struct.MyType
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini reopened this revision.
mantognini added a subscriber: ilya-biryukov.
mantognini added a comment.
This revision is now accepted and ready to land.

While investigating PR42665, I've noticed that `getImplicitObjectArgument` 
doesn't always return a pointer type. For example, when dealing with 
`shared_ptr`. In libc++'s test 
std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.pass.cpp,
 dumping `CE` in `EmitVirtualDestructorCall` shows that the callee is `.~Bar` 
(mind the `.`):

  CXXMemberCallExpr 0x29d10a0 'void'
  `-MemberExpr 0x29d1070 '' .~Bar 0x23f5250
`-CXXMemberCallExpr 0x29d1040 'struct Bar':'struct Bar' lvalue
  `-MemberExpr 0x29d1010 '' .second 0x2883328
`-MemberExpr 0x29d0f80 '__compressed_pair, struct Bar>':'class 
std::__1::__compressed_pair, struct Bar>' 
lvalue ->__data_ 0x2896960
  `-CXXThisExpr 0x29d0f70 'class std::__1::__shared_ptr_emplace > *' implicit this

However, for CodeGenCXX/virtual-pseudo-destructor-call.cpp in clang test suite, 
`CE` is this (mind the `->` this time):

  CXXMemberCallExpr 0x7787a8 'void'
  `-MemberExpr 0x778750 '' ->~A 0x778288
`-ImplicitCastExpr 0x778738 'struct A *' 
  `-DeclRefExpr 0x778708 'struct A *' lvalue ParmVar 0x778558 'a' 'struct A 
*'

Tomorrow, I'll try to reduce libc++ failing tests and add it to Clang's suite 
as part of this review. For now, I've run `check-all` and it seems that no test 
are failing anymore (with the patch I'm about to upload).

However, I'm not sure whether `getImplicitObjectArgument` is expected to have 
either pointer or non-pointer type. I guess it is, but, John, Ilya, could 
either of you confirm this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569



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

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 210605.
mantognini added a comment.

Add patch for Bug PR42665.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
  clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl

Index: clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS
+
+// This test ensures the proper address spaces and address space cast are used
+// for constructors, member functions and destructors.
+// See also atexit.cl and global_init.cl for other specific tests.
+
+// CHECK: %struct.MyType = type { i32 }
+struct MyType {
+  MyType(int i) : i(i) {}
+  MyType(int i) __constant : i(i) {}
+  ~MyType() {}
+  ~MyType() __constant {}
+  int bar() { return i + 2; }
+  int bar() __constant { return i + 1; }
+  int i;
+};
+
+// CHECK: @const1 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const1 = 1;
+// CHECK: @const2 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const2(2);
+// CHECK: @glob = addrspace(1) global %struct.MyType zeroinitializer
+MyType glob(1);
+
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
+// CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*), i32 1)
+
+// CHECK-LABEL: define spir_kernel void @fooGlobal()
+kernel void fooGlobal() {
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*))
+  glob.bar();
+  // CHECK: call i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* @const1)
+  const1.bar();
+  // CHECK: call void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* @const1)
+  const1.~MyType();
+}
+
+// CHECK-LABEL: define spir_kernel void @fooLocal()
+kernel void fooLocal() {
+  // CHECK: [[VAR:%.*]] = alloca %struct.MyType
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* [[REG]], i32 3)
+  MyType myLocal(3);
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* [[REG]])
+  myLocal.bar();
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* [[REG]])
+}
+
+// Ensure all members are defined for all the required address spaces.
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* %this)
Index: clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
===
--- clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
-
-struct MyType {
-  MyType(int i) : i(i) {}
-  MyType(int i) __constant : i(i) {}
-  int i;
-};
-
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
-__constant MyType const1 = 1;
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
-__constant MyType const2(2);
-//CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType ad

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini marked an inline comment as done.
mantognini added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1758-1764
+  if (CE) {
+ThisTy = CE->getImplicitObjectArgument()->getType();
+if (ThisTy->isPointerType())
+  ThisTy = ThisTy->getPointeeType();
+  } else {
+ThisTy = D->getDestroyedType();
+  }

This is the fix for PR42665. I've just realised that the same should be done in 
`MicrosoftCXXABI.cpp` -- will do that tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569



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

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 210850.
mantognini added a comment.

- Add minimal regression test for PR42665
- Add CXXMemberCallExpr::getObjectType()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CodeGenCXX/PR42665.cpp
  clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
  clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl

Index: clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS
+
+// This test ensures the proper address spaces and address space cast are used
+// for constructors, member functions and destructors.
+// See also atexit.cl and global_init.cl for other specific tests.
+
+// CHECK: %struct.MyType = type { i32 }
+struct MyType {
+  MyType(int i) : i(i) {}
+  MyType(int i) __constant : i(i) {}
+  ~MyType() {}
+  ~MyType() __constant {}
+  int bar() { return i + 2; }
+  int bar() __constant { return i + 1; }
+  int i;
+};
+
+// CHECK: @const1 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const1 = 1;
+// CHECK: @const2 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const2(2);
+// CHECK: @glob = addrspace(1) global %struct.MyType zeroinitializer
+MyType glob(1);
+
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
+// CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*), i32 1)
+
+// CHECK-LABEL: define spir_kernel void @fooGlobal()
+kernel void fooGlobal() {
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*))
+  glob.bar();
+  // CHECK: call i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* @const1)
+  const1.bar();
+  // CHECK: call void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* @const1)
+  const1.~MyType();
+}
+
+// CHECK-LABEL: define spir_kernel void @fooLocal()
+kernel void fooLocal() {
+  // CHECK: [[VAR:%.*]] = alloca %struct.MyType
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* [[REG]], i32 3)
+  MyType myLocal(3);
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* [[REG]])
+  myLocal.bar();
+  // CHECK: [[REG:%.*]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* [[REG]])
+}
+
+// Ensure all members are defined for all the required address spaces.
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* %this)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* %this)
Index: clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
===
--- clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
-
-struct MyType {
-  MyType(int i) : i(i) {}
-  MyType(int i) __constant : i(i) {}
-  int i;
-};
-
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
-__constant MyType const1 = 1;
-//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
-__constant My

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D64569#1592059 , @rjmccall wrote:

> Yes, that's the right fix, although you might also consider adding a 
> `getObjectType()` to `CXXMemberCallExpr`.


Thanks, John, it should be better now, but let me know if I can improve 
something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569



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

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

FYI In order to fix buildbot test failures, I've pushed 
https://reviews.llvm.org/rG1b2da771f561affe36eb5eb0c7a3d2862c5a5c1c. I'll keep 
an eye on buildbots for additional fallout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569



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

[PATCH] D65286: [OpenCL] Allow OpenCL C style vector initialization in C++

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In `vector_literals_nested.cl`, we have tests for (global) constants. Do you 
think it would be worth testing those as well in C++ mode? Maybe the two files 
(`vector_literals_nested.cl` and `vector_literals_valid.cl`) should also be 
merged as most of their content seems duplicated.

In C++, we have the comma operator and therefore the AST is significantly 
different. Running the content of your test file with `-x c++` shows that it is 
rejected as desired. It could be worth having a negative test to ensure we 
always reject this in vanilla C++.

Regarding the code itself, I'm not familiar with 
`InitializationSequence`/`InitListChecker` that much, but the patch makes sense 
I would say.


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

https://reviews.llvm.org/D65286



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

[PATCH] D65286: [OpenCL] Allow OpenCL C style vector initialization in C++

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for the update.


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

https://reviews.llvm.org/D65286



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

[PATCH] D66294: [Docs][OpenCL] Release 9.0 notes for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini requested changes to this revision.
mantognini added a comment.
This revision now requires changes to proceed.

The overall structure seems alright. I'll let people more familiar with the 
code-base judge whether other important features should be added there as well.




Comment at: docs/ReleaseNotes.rst:49
 
+- Full experimental support of :ref:`C++ for OpenCL ` has
+  been added.

"Full experimental" feels like an oxymoron. I would drop the "full".



Comment at: docs/ReleaseNotes.rst:49
 
+- Full experimental support of :ref:`C++ for OpenCL ` has
+  been added.

mantognini wrote:
> "Full experimental" feels like an oxymoron. I would drop the "full".
"support of" -> "support for"



Comment at: docs/ReleaseNotes.rst:138
 
+- Support of address space attribute in various C++ features was improved, 
refer
+  to :ref:`C++ for OpenCL ` for more details). The following 
features

Support //for// address space attribute//s// [...] improved //(refer// [...]

Alternatively, if you don't want to use parenthesis, drop the closing 
parenthesis on the next line.



Comment at: docs/ReleaseNotes.rst:142
+
+  (1) address spaces as method qualifiers are not accepted yet;
+

Inconsistent sentence capitalization between the two bullet points.



Comment at: docs/ReleaseNotes.rst:173
+
+- Added initial support for implicitly including OpenCL BIFs using
+  efficient trie lookup generated by TableGen. A corresponding

If the BIF acronym wasn't introduced before, it should be replaced with 
"builtin functions". It seems we don't have more file context in this review so 
I cannot tell.



Comment at: docs/ReleaseNotes.rst:175
+  efficient trie lookup generated by TableGen. A corresponding
+  frontend only flag ``-fadd-opencl-builtins`` has been added to
+  enable trie during parsing.

I'm not 100% sure about the grammar rule in English, but shouldn't there be a 
"-" between "frontend" and "only" here to make it an adjective-ish?



Comment at: docs/ReleaseNotes.rst:183
+
+- Simplified representation of blocks including their generation in
+  IR i.e. indirect calls to block function has been changed to

Simplified //the// representation of blocks//**,**// including their generation 
//into// IR. //Furthermore,// indirect calls [...]

(I'm assuming here that the indirect calls to block function is not the only 
improvement. And even if it is, "i.e." is less impressive, isn't it?)



Comment at: docs/ReleaseNotes.rst:194
+
+- Improved math builtin function of long long for x86.
+

Maybe this would be better?

Improved math builtin functions with parameters of type `long long` for x86.





Comment at: docs/ReleaseNotes.rst:201
+
+Full experimental support for C++17 features in OpenCL has been
+added and backwards compatibility to OpenCL C v2.0 was enabled.

Ditto, I would drop "full" here.



Comment at: docs/ReleaseNotes.rst:202
+Full experimental support for C++17 features in OpenCL has been
+added and backwards compatibility to OpenCL C v2.0 was enabled.
+The documentation has been added for supported language features

compatible //with//



Comment at: docs/ReleaseNotes.rst:209
+
+- Templates parameters and arguments
+

Did you meant to indent this bullet point as well?



Comment at: docs/ReleaseNotes.rst:215
+
+  - Objects and member functions including special member
+functions;

Missing comma: "functions, including"



Comment at: docs/ReleaseNotes.rst:220
+
+  - Method qualifiers are allowed with address spaces;
+

Maybe something along these line would be better?

Methods can be overloaded for different address spaces.

Or, if you want to emphasis the qualifiers,

Method qualifiers now include address space.



Comment at: docs/ReleaseNotes.rst:222
+
+  - Address space deduction has been extended for C++ use cases;
+

This seems to be already included in previous point.



Comment at: docs/ReleaseNotes.rst:226
+
+  - Cast operators are now preventing to converting address
+spaces. They can still be cast using C style cast.

Which "cast" operators?



Comment at: docs/ReleaseNotes.rst:226
+
+  - Cast operators are now preventing to converting address
+spaces. They can still be cast using C style cast.

mantognini wrote:
> Which "cast" operators?
[...] are now prevent//ed from// converting [...]



Comment at: docs/ReleaseNotes.rst:229
+
+- Vector types as in OpenCL C including compound vector
+  initialization.

missing comma: "C, including"



C

[PATCH] D66294: [Docs][OpenCL] Release 9.0 notes for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.



Comment at: docs/ReleaseNotes.rst:209
+Implemented features are:
+- Address space behavior is improved in majority of C++ features:
+

I think Sphinx/RST wants an empty line here.

Nitpicking for consistency, could you have a `;` at then end of each bullet 
point (except the last one, obviously)? Thanks.


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

https://reviews.llvm.org/D66294



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

[PATCH] D66294: [Docs][OpenCL] Release 9.0 notes for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

Thanks for addressing my comments.


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

https://reviews.llvm.org/D66294



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

[PATCH] D64074: [OpenCL][Sema] Fix builtin rewriting

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
mantognini added reviewers: rjmccall, Anastasia.
Herald added subscribers: cfe-commits, yaxunl.
Herald added a project: clang.

This patch ensures built-in functions are rewritten using the proper
parent declaration. Tests are added to ensure the functionality works
also with C++ for OpenCL.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64074

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGenOpenCL/builtins.cl
  clang/test/CodeGenOpenCL/pipe_builtin.cl
  clang/test/CodeGenOpenCL/to_addr_builtin.cl


Index: clang/test/CodeGenOpenCL/to_addr_builtin.cl
===
--- clang/test/CodeGenOpenCL/to_addr_builtin.cl
+++ clang/test/CodeGenOpenCL/to_addr_builtin.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=CL2.0 
-o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=c++ -o 
- %s | FileCheck %s
 
 // CHECK: %[[A:.*]] = type { float, float, float }
 typedef struct {
Index: clang/test/CodeGenOpenCL/pipe_builtin.cl
===
--- clang/test/CodeGenOpenCL/pipe_builtin.cl
+++ clang/test/CodeGenOpenCL/pipe_builtin.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_subgroups -O0 -cl-std=CL2.0 -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_subgroups -O0 -cl-std=c++ -o - 
%s | FileCheck %s
 
 // CHECK-DAG: %opencl.pipe_ro_t = type opaque
 // CHECK-DAG: %opencl.pipe_wo_t = type opaque
Index: clang/test/CodeGenOpenCL/builtins.cl
===
--- clang/test/CodeGenOpenCL/builtins.cl
+++ clang/test/CodeGenOpenCL/builtins.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o 
- -triple "spir-unknown-unknown" | FileCheck %s
+// RUN: %clang_cc1 %s -finclude-default-header -cl-std=c++ -fblocks -O0 
-emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
 
 void testBranchingOnEnqueueKernel(queue_t default_queue, unsigned flags, 
ndrange_t ndrange) {
 // Ensure `enqueue_kernel` can be branched upon.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5360,7 +5360,7 @@
 ///  FunctionDecl is returned.
 /// TODO: Handle pointer return types.
 static FunctionDecl *rewriteBuiltinFunctionDecl(Sema *Sema, ASTContext 
&Context,
-const FunctionDecl *FDecl,
+FunctionDecl *FDecl,
 MultiExprArg ArgExprs) {
 
   QualType DeclType = FDecl->getType();
@@ -5408,7 +5408,7 @@
   FunctionProtoType::ExtProtoInfo EPI;
   QualType OverloadTy = Context.getFunctionType(FT->getReturnType(),
 OverloadParams, EPI);
-  DeclContext *Parent = Context.getTranslationUnitDecl();
+  DeclContext *Parent = FDecl->getParent();
   FunctionDecl *OverloadDecl = FunctionDecl::Create(Context, Parent,
 FDecl->getLocation(),
 FDecl->getLocation(),


Index: clang/test/CodeGenOpenCL/to_addr_builtin.cl
===
--- clang/test/CodeGenOpenCL/to_addr_builtin.cl
+++ clang/test/CodeGenOpenCL/to_addr_builtin.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=c++ -o - %s | FileCheck %s
 
 // CHECK: %[[A:.*]] = type { float, float, float }
 typedef struct {
Index: clang/test/CodeGenOpenCL/pipe_builtin.cl
===
--- clang/test/CodeGenOpenCL/pipe_builtin.cl
+++ clang/test/CodeGenOpenCL/pipe_builtin.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_subgroups -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_subgroups -O0 -cl-std=c++ -o - %s | FileCheck %s
 
 // CHECK-DAG: %opencl.pipe_ro_t = type opaque
 // CHECK-DAG: %opencl.pipe_wo_t = type opaque
Index: clang/test/CodeGenOpenCL/builtins.cl
===
--- clang/test/CodeGenOpenCL/builtins.cl
+++ clang/test/CodeGenOpenCL/builtins.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
+// RUN: %clang_cc1 %s -finclude-default-header -cl-std=c++ -fblocks -O0 -emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
 
 void testBranchingOnEnqueueKernel(queue_t default_queue, unsigned flags, ndrange_t ndrange) {
 // Ensure `enqueue_kernel` can be branched 

[PATCH] D64074: [OpenCL][Sema] Fix builtin rewriting

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 207569.
mantognini added a comment.

Address comments: reduce testing & add FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64074

Files:
  clang/include/clang/Basic/Builtins.def
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGenOpenCL/builtins.cl
  clang/test/CodeGenOpenCL/pipe_builtin.cl
  clang/test/CodeGenOpenCL/to_addr_builtin.cl


Index: clang/test/CodeGenOpenCL/to_addr_builtin.cl
===
--- clang/test/CodeGenOpenCL/to_addr_builtin.cl
+++ clang/test/CodeGenOpenCL/to_addr_builtin.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=CL2.0 
-o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=c++ -o 
- %s | FileCheck %s
 
 // CHECK: %[[A:.*]] = type { float, float, float }
 typedef struct {
Index: clang/test/CodeGenOpenCL/pipe_builtin.cl
===
--- clang/test/CodeGenOpenCL/pipe_builtin.cl
+++ clang/test/CodeGenOpenCL/pipe_builtin.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_subgroups -O0 -cl-std=CL2.0 -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_subgroups -O0 -cl-std=c++ -o - 
%s | FileCheck %s
 
 // CHECK-DAG: %opencl.pipe_ro_t = type opaque
 // CHECK-DAG: %opencl.pipe_wo_t = type opaque
Index: clang/test/CodeGenOpenCL/builtins.cl
===
--- clang/test/CodeGenOpenCL/builtins.cl
+++ clang/test/CodeGenOpenCL/builtins.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o 
- -triple "spir-unknown-unknown" | FileCheck %s
+// RUN: %clang_cc1 %s -finclude-default-header -cl-std=c++ -fblocks -O0 
-emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
 
 void testBranchingOnEnqueueKernel(queue_t default_queue, unsigned flags, 
ndrange_t ndrange) {
 // Ensure `enqueue_kernel` can be branched upon.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5360,7 +5360,7 @@
 ///  FunctionDecl is returned.
 /// TODO: Handle pointer return types.
 static FunctionDecl *rewriteBuiltinFunctionDecl(Sema *Sema, ASTContext 
&Context,
-const FunctionDecl *FDecl,
+FunctionDecl *FDecl,
 MultiExprArg ArgExprs) {
 
   QualType DeclType = FDecl->getType();
@@ -5408,7 +5408,7 @@
   FunctionProtoType::ExtProtoInfo EPI;
   QualType OverloadTy = Context.getFunctionType(FT->getReturnType(),
 OverloadParams, EPI);
-  DeclContext *Parent = Context.getTranslationUnitDecl();
+  DeclContext *Parent = FDecl->getParent();
   FunctionDecl *OverloadDecl = FunctionDecl::Create(Context, Parent,
 FDecl->getLocation(),
 FDecl->getLocation(),
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -1477,6 +1477,7 @@
 BUILTIN(__builtin_coro_end, "bv*Ib", "n")
 BUILTIN(__builtin_coro_suspend, "cIb", "n")
 BUILTIN(__builtin_coro_param, "bv*v*", "n")
+
 // OpenCL v2.0 s6.13.16, s9.17.3.5 - Pipe functions.
 // We need the generic prototype, since the packet type could be anything.
 LANGBUILTIN(read_pipe, "i.", "tn", OCLC20_LANG)
@@ -1512,6 +1513,8 @@
 LANGBUILTIN(get_kernel_sub_group_count_for_ndrange, "Ui.", "tn", OCLC20_LANG)
 
 // OpenCL v2.0 s6.13.9 - Address space qualifier functions.
+// FIXME Pointer parameters of OpenCL builtins should have their address space
+// requirement defined.
 LANGBUILTIN(to_global, "v*v*", "tn", OCLC20_LANG)
 LANGBUILTIN(to_local, "v*v*", "tn", OCLC20_LANG)
 LANGBUILTIN(to_private, "v*v*", "tn", OCLC20_LANG)


Index: clang/test/CodeGenOpenCL/to_addr_builtin.cl
===
--- clang/test/CodeGenOpenCL/to_addr_builtin.cl
+++ clang/test/CodeGenOpenCL/to_addr_builtin.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=c++ -o - %s | FileCheck %s
 
 // CHECK: %[[A:.*]] = type { float, float, float }
 typedef struct {
Index: clang/test/CodeGenOpenCL/pipe_builtin.cl
===
--- clang/test/CodeGenOpenCL/pipe_builtin.cl
+++ clang/test/CodeGenOpenCL/pipe_builtin.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr

[PATCH] D64074: [OpenCL][Sema] Fix builtin rewriting

[Marco Antognini via Phabricator via cfe-commits]

mantognini marked an inline comment as done.
mantognini added a comment.

I added the FIXME and reduced the amount of testing. Let me know if it looks 
alright.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64074



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

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
mantognini added reviewers: rjmccall, Anastasia.
Herald added subscribers: cfe-commits, yaxunl.
Herald added a project: clang.

This patch ensures that the following code is compiled identically with
-cl-std=CL2.0 and -fblocks -cl-std=c++.

  kernel void test(void) {
void (^const block_A)(void) = ^{
  return;
};
  }

A new test is not added because cl20-device-side-enqueue.cl will cover
this once blocks are further improved for C++ for OpenCL.

The changes to Sema::PerformImplicitConversion are based on
the parts of Sema::CheckAssignmentConstraints on block pointer
conversions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64083

Files:
  clang/lib/Sema/SemaExprCXX.cpp


Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -4216,7 +4216,20 @@
 break;
 
   case ICK_Block_Pointer_Conversion: {
-From = ImpCastExprToType(From, ToType.getUnqualifiedType(), CK_BitCast,
+QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
+QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
+
+// Assumptions based on Sema::IsBlockPointerConversion.
+assert(isa(LHSType) && "BlockPointerType expected");
+assert(isa(RHSType) && "BlockPointerType expected");
+
+LangAS AddrSpaceL =
+LHSType->getAs()->getPointeeType().getAddressSpace();
+LangAS AddrSpaceR =
+RHSType->getAs()->getPointeeType().getAddressSpace();
+CastKind Kind =
+AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
+From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,
  VK_RValue, /*BasePath=*/nullptr, CCK).get();
 break;
   }


Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -4216,7 +4216,20 @@
 break;
 
   case ICK_Block_Pointer_Conversion: {
-From = ImpCastExprToType(From, ToType.getUnqualifiedType(), CK_BitCast,
+QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
+QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
+
+// Assumptions based on Sema::IsBlockPointerConversion.
+assert(isa(LHSType) && "BlockPointerType expected");
+assert(isa(RHSType) && "BlockPointerType expected");
+
+LangAS AddrSpaceL =
+LHSType->getAs()->getPointeeType().getAddressSpace();
+LangAS AddrSpaceR =
+RHSType->getAs()->getPointeeType().getAddressSpace();
+CastKind Kind =
+AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
+From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,
  VK_RValue, /*BasePath=*/nullptr, CCK).get();
 break;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64074: [OpenCL][Sema] Fix builtin rewriting

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 207994.
mantognini added a comment.

Addressed missing colon in FIXME comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64074

Files:
  clang/include/clang/Basic/Builtins.def
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGenOpenCL/builtins.cl
  clang/test/CodeGenOpenCL/pipe_builtin.cl
  clang/test/CodeGenOpenCL/to_addr_builtin.cl


Index: clang/test/CodeGenOpenCL/to_addr_builtin.cl
===
--- clang/test/CodeGenOpenCL/to_addr_builtin.cl
+++ clang/test/CodeGenOpenCL/to_addr_builtin.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=CL2.0 
-o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=c++ -o 
- %s | FileCheck %s
 
 // CHECK: %[[A:.*]] = type { float, float, float }
 typedef struct {
Index: clang/test/CodeGenOpenCL/pipe_builtin.cl
===
--- clang/test/CodeGenOpenCL/pipe_builtin.cl
+++ clang/test/CodeGenOpenCL/pipe_builtin.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_subgroups -O0 -cl-std=CL2.0 -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_subgroups -O0 -cl-std=c++ -o - 
%s | FileCheck %s
 
 // CHECK-DAG: %opencl.pipe_ro_t = type opaque
 // CHECK-DAG: %opencl.pipe_wo_t = type opaque
Index: clang/test/CodeGenOpenCL/builtins.cl
===
--- clang/test/CodeGenOpenCL/builtins.cl
+++ clang/test/CodeGenOpenCL/builtins.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o 
- -triple "spir-unknown-unknown" | FileCheck %s
+// RUN: %clang_cc1 %s -finclude-default-header -cl-std=c++ -fblocks -O0 
-emit-llvm -o - -triple "spir-unknown-unknown" | FileCheck %s
 
 void testBranchingOnEnqueueKernel(queue_t default_queue, unsigned flags, 
ndrange_t ndrange) {
 // Ensure `enqueue_kernel` can be branched upon.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5360,7 +5360,7 @@
 ///  FunctionDecl is returned.
 /// TODO: Handle pointer return types.
 static FunctionDecl *rewriteBuiltinFunctionDecl(Sema *Sema, ASTContext 
&Context,
-const FunctionDecl *FDecl,
+FunctionDecl *FDecl,
 MultiExprArg ArgExprs) {
 
   QualType DeclType = FDecl->getType();
@@ -5408,7 +5408,7 @@
   FunctionProtoType::ExtProtoInfo EPI;
   QualType OverloadTy = Context.getFunctionType(FT->getReturnType(),
 OverloadParams, EPI);
-  DeclContext *Parent = Context.getTranslationUnitDecl();
+  DeclContext *Parent = FDecl->getParent();
   FunctionDecl *OverloadDecl = FunctionDecl::Create(Context, Parent,
 FDecl->getLocation(),
 FDecl->getLocation(),
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -1477,6 +1477,7 @@
 BUILTIN(__builtin_coro_end, "bv*Ib", "n")
 BUILTIN(__builtin_coro_suspend, "cIb", "n")
 BUILTIN(__builtin_coro_param, "bv*v*", "n")
+
 // OpenCL v2.0 s6.13.16, s9.17.3.5 - Pipe functions.
 // We need the generic prototype, since the packet type could be anything.
 LANGBUILTIN(read_pipe, "i.", "tn", OCLC20_LANG)
@@ -1512,6 +1513,8 @@
 LANGBUILTIN(get_kernel_sub_group_count_for_ndrange, "Ui.", "tn", OCLC20_LANG)
 
 // OpenCL v2.0 s6.13.9 - Address space qualifier functions.
+// FIXME: Pointer parameters of OpenCL builtins should have their address space
+// requirement defined.
 LANGBUILTIN(to_global, "v*v*", "tn", OCLC20_LANG)
 LANGBUILTIN(to_local, "v*v*", "tn", OCLC20_LANG)
 LANGBUILTIN(to_private, "v*v*", "tn", OCLC20_LANG)


Index: clang/test/CodeGenOpenCL/to_addr_builtin.cl
===
--- clang/test/CodeGenOpenCL/to_addr_builtin.cl
+++ clang/test/CodeGenOpenCL/to_addr_builtin.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple spir-unknown-unknown -emit-llvm -O0 -cl-std=c++ -o - %s | FileCheck %s
 
 // CHECK: %[[A:.*]] = type { float, float, float }
 typedef struct {
Index: clang/test/CodeGenOpenCL/pipe_builtin.cl
===
--- clang/test/CodeGenOpenCL/pipe_builtin.cl
+++ clang/test/CodeGenOpenCL/pipe_builtin.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm -cl-ext=+cl_khr_su

[PATCH] D64074: [OpenCL][Sema] Fix builtin rewriting

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

I'll commit this along with D64083  in a few 
days in case John has any feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64074



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

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365500: [OpenCL][Sema] Improve address space support for 
blocks (authored by mantognini, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64083?vs=207573&id=208683#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64083

Files:
  cfe/trunk/lib/Sema/SemaExprCXX.cpp


Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -4216,7 +4216,20 @@
 break;
 
   case ICK_Block_Pointer_Conversion: {
-From = ImpCastExprToType(From, ToType.getUnqualifiedType(), CK_BitCast,
+QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
+QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
+
+// Assumptions based on Sema::IsBlockPointerConversion.
+assert(isa(LHSType) && "BlockPointerType expected");
+assert(isa(RHSType) && "BlockPointerType expected");
+
+LangAS AddrSpaceL =
+LHSType->getAs()->getPointeeType().getAddressSpace();
+LangAS AddrSpaceR =
+RHSType->getAs()->getPointeeType().getAddressSpace();
+CastKind Kind =
+AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
+From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,
  VK_RValue, /*BasePath=*/nullptr, CCK).get();
 break;
   }


Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -4216,7 +4216,20 @@
 break;
 
   case ICK_Block_Pointer_Conversion: {
-From = ImpCastExprToType(From, ToType.getUnqualifiedType(), CK_BitCast,
+QualType LHSType = Context.getCanonicalType(ToType).getUnqualifiedType();
+QualType RHSType = Context.getCanonicalType(FromType).getUnqualifiedType();
+
+// Assumptions based on Sema::IsBlockPointerConversion.
+assert(isa(LHSType) && "BlockPointerType expected");
+assert(isa(RHSType) && "BlockPointerType expected");
+
+LangAS AddrSpaceL =
+LHSType->getAs()->getPointeeType().getAddressSpace();
+LangAS AddrSpaceR =
+RHSType->getAs()->getPointeeType().getAddressSpace();
+CastKind Kind =
+AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion : CK_BitCast;
+From = ImpCastExprToType(From, ToType.getUnqualifiedType(), Kind,
  VK_RValue, /*BasePath=*/nullptr, CCK).get();
 break;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64418: [Docs][OpenCL] Documentation of C++ for OpenCL mode

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Here are a few comments from me but keep in mind that English is not my primary 
language




Comment at: docs/LanguageExtensions.rst:1562-1565
+C style cast will follow OpenCL C v2.0 rules (s6.5.5). All cast operators will
+permit implicit conversion to generic. However converting from named address
+spaces to generic can only be done using ``addrspace_cast``. Note that
+conversions between ``__constant`` and any other is still disallowed.

I would suggest using `__generic` or //generic address space//. The later is a 
bit more verbose, but makes the sentence more natural I think.



Comment at: docs/LanguageExtensions.rst:1582
+  void foo() {
+T m; // address space of m will be knowns at template instantiation time.
+T * ptr; // ptr points to generic address space object.

s/knowns/known/



Comment at: docs/LanguageExtensions.rst:1611-1615
+By default references will refer to generic address space objects (except for
+dependent types that are not template specializations
+(:ref:``). When references are bound to values 
address
+space compatibility check will be performed. The logic will largely follow the 
rules
+from address space pointer conversion (OpenCL v2.0 s6.5.5).

The `ref` is broken. There's also one too many `(`.



Comment at: docs/LanguageExtensions.rst:1613-1614
+dependent types that are not template specializations
+(:ref:``). When references are bound to values 
address
+space compatibility check will be performed. The logic will largely follow the 
rules
+from address space pointer conversion (OpenCL v2.0 s6.5.5).

> When references are bound to values address space compatibility check will be 
> performed.

This might just be me so this might actually be okay, but I feel the sentence 
is simpler when rewritten as "Address space compatibility checks are performed 
when references are bound to values." What do you think?

> The logic will largely follow the rules from address space pointer conversion

Present tense should probably be better. Are there actually any differences? If 
no, I would drop "largely" from the sentence. Otherwise it might be good to 
actually list the differences.



Comment at: docs/LanguageExtensions.rst:1617
+
+**Default AS**
+ 

AS might be better fully spelled out.



Comment at: docs/LanguageExtensions.rst:1619
+ 
+All non-static methods take implicit object parameter that is a pointer type. 
By
+default this pointer parameter is in generic address space. All concrete 
objects

take //an// implicit



Comment at: docs/LanguageExtensions.rst:1624
+address space won't be compiled unless address space is explicitly specified 
using
+address space method qualifiers ((:ref:``) 
as the
+conversion between ``__constant`` and generic is disallowed. Method qualifiers 
can

`ref` is broken and there's one too manu `(`.



Comment at: docs/LanguageExtensions.rst:1627
+also be useful in case conversion to generic address space is undesirable 
(even if
+it's legal). For example if we need to take advantage of memory bank accesses.
+Please note this not only applies to regular methods but to constructors and

> For example if we need to take advantage of memory bank accesses.

Maybe "For example: to take advantage [...]"?



Comment at: docs/LanguageExtensions.rst:1662
+
+  class C{
+// Has the following implicit definition

nitpicking: missing space before `{`.



Comment at: docs/LanguageExtensions.rst:1671
+
+**Built in operators**
+

Typo: //Builtin//



Comment at: docs/LanguageExtensions.rst:1673
+
+All builtin operators will be available in the specific address spaces, thus 
no conversion
+to generic is performed.

"specific" seems to imply to the user will have to explicitly write down the 
AS. Maybe "requested" or "desired" would fit better?



Comment at: docs/LanguageExtensions.rst:1679
+There is no deduction of address spaces in non-pointer/non-reference template 
parameters and
+dependent types (:ref:``). The address space of 
template
+parameter is deduced during the type deduction if it's not explicitly provided 
in

The reference seems broken as well.



Comment at: docs/LanguageExtensions.rst:1708
+  void bar() {
+foo3<__global int>(); // error: conflicting address space qualifiers are 
provided
+  }

s/foo3/foo/



Comment at: docs/LanguageExtensions.rst:1717
+  void foo(){
+  T var;
+  }

nitpicking: indentation. Might as well rename it to `ii` to match the other 
example.



Comment at: docs/LanguageExtensions.rst:1753-1761
+Global objects are constructe

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: cfe-commits, Anastasia, yaxunl.
Herald added a project: clang.

This patch does mainly three things:

1. It fixes a false positive error detection in Sema that is similar to D62156 
. The error happens when explicitly calling an 
overloaded destructor for different address spaces.
2. It selects the correct destructor when multiple overloads for address spaces 
are available.
3. It inserts the expected address space cast when invoking a destructor, if 
needed, and therefore fixes a crash due to the unmet assertion in 
llvm::CastInst::Create.

The following is a reproducer of the three issues:

  struct MyType {
~MyType() {}
~MyType() __constant {}
  };
  
  __constant MyType myGlobal{};
  
  kernel void foo() {
myGlobal.~MyType(); // 1 and 2.
// 1. error: cannot initialize object parameter of type
//'__generic MyType' with an expression of type '__constant MyType'
// 2. error: no matching member function for call to '~MyType'
  }
  
  kernel void bar() {
// 3. The implicit call to the destructor crashes due to:
//Assertion `castIsValid(op, S, Ty) && "Invalid cast!"' failed.
//in llvm::CastInst::Create.
MyType myLocal;
  }

The added test depends on D62413  and covers a 
few more things than the
above reproducer.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64569

Files:
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CodeGenOpenCLCXX/addrspace-ctor.cl
  clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl

Index: clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
===
--- /dev/null
+++ clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS
+
+// This test ensures the proper address spaces and address space cast are used
+// for constructors, member functions and destructors.
+// See also atexit.cl and global_init.cl for other specific tests.
+
+// CHECK: %struct.MyType = type { i32 }
+struct MyType {
+  MyType(int i) : i(i) {}
+  MyType(int i) __constant : i(i) {}
+  ~MyType() {}
+  ~MyType() __constant {}
+  int bar() { return i + 2; }
+  int bar() __constant { return i + 1; }
+  int i;
+};
+
+// CHECK: @const1 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const1 = 1;
+// CHECK: @const2 = addrspace(2) global %struct.MyType zeroinitializer
+__constant MyType const2(2);
+// CHECK: @glob = addrspace(1) global %struct.MyType zeroinitializer
+MyType glob(1);
+
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
+// CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
+// CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*), i32 1)
+
+// CHECK-LABEL: define spir_kernel void @fooGlobal()
+kernel void fooGlobal() {
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*))
+  glob.bar();
+  // CHECK: call i32 @_ZNU3AS26MyType3barEv(%struct.MyType addrspace(2)* @const1)
+  const1.bar();
+  // CHECK: call void @_ZNU3AS26MyTypeD1Ev(%struct.MyType addrspace(2)* @const1)
+  const1.~MyType();
+}
+
+// CHECK-LABEL: define spir_kernel void @fooLocal()
+kernel void fooLocal() {
+  // CHECK: [[VAR:%.*]] = alloca %struct.MyType
+  // CHECK: [[REG:%[0-9]+]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* [[REG]], i32 3)
+  MyType myLocal(3);
+  // CHECK: [[REG:%[0-9]+]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call i32 @_ZNU3AS46MyType3barEv(%struct.MyType addrspace(4)* [[REG]])
+  myLocal.bar();
+  // CHECK: [[REG:%[0-9]+]] = addrspacecast %struct.MyType* [[VAR]] to %struct.MyType addrspace(4)*
+  // CHECK: call void @_ZNU3AS46MyTypeD1Ev(%struct.MyType addrspace(4)* [[REG]])
+}
+
+// Ensure all members are defined for all the required address spaces.
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* %this, i32 %i)
+// CHECK-DEFINITIONS-DAG: define linkonce_odr void @_ZNU3AS26MyTypeD1Ev(%struct.MyType 

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

[Marco Antognini via Phabricator via cfe-commits]

mantognini added reviewers: Anastasia, rjmccall.
mantognini marked 6 inline comments as done.
mantognini added inline comments.



Comment at: clang/lib/CodeGen/CGClass.cpp:2016
   CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
-/*Delegating=*/false, addr);
+/*Delegating=*/false, addr, type);
 }

This is the only place where the new parameter has an interesting value. In the 
~10 other calls to `EmitCXXDestructorCall` overloads, the default value of this 
new parameter is used instead.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:96
+llvm::Value *ImplicitParam, QualType ImplicitParamTy, const CallExpr *CE,
+QualType ThisTy) {
+  const CXXMethodDecl *DtorDecl = cast(Dtor.getDecl());

This new parameter is required in order to call `performAddrSpaceCast`.



Comment at: clang/lib/CodeGen/CGExprCXX.cpp:117-118
+  llvm::Type *NewType = CGM.getTypes().ConvertType(DstTy);
+  This = getTargetHooks().performAddrSpaceCast(*this, This, SrcAS, DstAS,
+   NewType);
+}

This is effectively the fix for the third point mentioned in the description.

Alternatively, `This = Builder.CreatePointerBitCastOrAddrSpaceCast(This, 
NewType);` seems to work equally well and does not require the extra new 
parameter.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8428-8439
+bool DiagOccured = false;
 FTI.MethodQualifiers->forEachQualifier(
 [&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) {
+  // This diagnostic should be emitted on any qualifier except an addr
+  // space qualifier. However, forEachQualifier currently doesn't visit
+  // addr space qualifiers, so there's no way to write this condition
+  // right now; we just diagnose on everything.

This is the fix for the first point in the description.



Comment at: clang/lib/Sema/SemaOverload.cpp:5092-5097
-  Qualifiers Quals;
+  Qualifiers Quals = Method->getMethodQualifiers();
   if (isa(Method)) {
 Quals.addConst();
 Quals.addVolatile();
-  } else {
-Quals = Method->getMethodQualifiers();

This is the fix for the second point in the description.



Comment at: clang/test/CodeGenOpenCLCXX/addrspace-with-class.cl:1
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 
-o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 
-o - | FileCheck %s --check-prefix=CHECK-DEFINITIONS

This test extends `addrspace-ctor.cl`, which is therefore deleted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64569



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

[PATCH] D62591: [OpenCL][PR42031] Prevent deducing addr space in type alias.

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.



Comment at: lib/Sema/SemaType.cpp:7355
+  // object.
+  (D.getContext() == DeclaratorContext::AliasDeclContext) ||
   // Do not deduce addr space for types used to define a typedef and the

I think this should also check for `!IsPointee` as it breaks the following 
example with `error: cannot initialize a variable of type 'c_ptr' (aka 'c1 *') 
with an rvalue of type 'c1 *'`.


```
struct c1 { int x; };
using c_ptr = c1*;

__kernel void test2() {
  c1 x = { 0 };
  c1* y = &x; // okay
  c_ptr z = &x; // fails
}
```



Comment at: test/SemaOpenCLCXX/address-space-deduction.cl:1
 //RUN: %clang_cc1 %s -cl-std=c++ -pedantic -ast-dump -verify
 

I guess it's an oversight from previous reviews, but FileCheck is not called 
here.



Comment at: test/SemaOpenCLCXX/address-space-deduction.cl:5
 
 //CHECK: |-VarDecl  foo {{.*}} 'const __global int' constexpr cinit
 constexpr int foo = 0;

The pattern should probably be:

`//CHECK: |-VarDecl {{.*}} foo 'const __global int' constexpr cinit`

(i.e. move the {{.*}} before foo).



Comment at: test/SemaOpenCLCXX/address-space-deduction.cl:10
 public:
   //CHECK: `-VarDecl {{.*}} foo2 'const __global int' static constexpr cinit
   static constexpr int foo2 = 0;

This check fails for me because `inline` is "missing" between `static` and 
`constexpr`. Not sure if it makes sense for `foo2` to be inline or not, but 
that's a different story.


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

https://reviews.llvm.org/D62591



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

[PATCH] D69072: [OpenCL] Added doc to describe OpenCL support

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Shouldn't this page be referenced from `clang/docs/index.rst`?

In the long run, how would this page differ from 
https://releases.llvm.org/9.0.0/tools/clang/docs/LanguageExtensions.html#opencl-features
 and 
https://releases.llvm.org/9.0.0/tools/clang/docs/UsersManual.html#opencl-features
 ? Do you think the content of those pages should be (partially) moved to the 
new one?


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

https://reviews.llvm.org/D69072



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

[PATCH] D69072: [OpenCL] Added doc to describe OpenCL support

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

Alright, thanks for the explanation. LGTM then.


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

https://reviews.llvm.org/D69072



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

[PATCH] D60193: [OpenCL] Added addrspace_cast operator

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Thanks for your clarifications and updates. Just one tiny question about a test 
file, but otherwise LGTM.




Comment at: clang/test/SemaOpenCLCXX/addrspace_cast.cl:19-24
+template 
+void test_temp(__global int *par) {
+  T *var1 = addrspace_cast(par);
+  __private T *var2 = addrspace_cast<__private T *>(par);
+  T var3 = addrspace_cast(par);
+}

Does this test anything since it's not instantiated?



Comment at: lib/Sema/SemaCast.cpp:285
+return Op.complete(CXXAddrspaceCastExpr::Create(Context, Op.ResultType,
+Op.ValueKind, Op.SrcExpr.get(),
+  DestTInfo,

Anastasia wrote:
> mantognini wrote:
> > The formatting looks a bit funny here.
> I agree, I just matched the style of the old code to keep it coherent. 
> Although perhaps I should rather adhere to the current style?
I don't have a strong opinion on what's best.



Comment at: lib/Sema/SemaCast.cpp:2338
   auto DestPointeeType = DestPtrType->getPointeeType();
   if (SrcPointeeType.getAddressSpace() == DestPointeeType.getAddressSpace())
 return TC_NotApplicable;

Anastasia wrote:
> mantognini wrote:
> > Wouldn't this limit usage of the cast unnecessarily? I'm thinking this 
> > could be transformed to a NOP, which could be beneficial to make (template) 
> > code simpler to write.
> I am not sure what you mean. I have added the test for templates and it 
> caught a bug in lib/AST/Expr.cpp with assert condition.
> 
> However, now that I think about this more, I believe we should allow 
> compiling this?
> 
> ```
> __private int* i;
> addrspace_cast(i);
> ```
> 
> Currently it outputs an error.
Yes, that's what I meant. (Although I see it's not part of this review so I'm 
not saying this should be changed now.)


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

https://reviews.llvm.org/D60193



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

[PATCH] D60193: [OpenCL] Added addrspace_cast operator

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/test/SemaOpenCLCXX/addrspace_cast.cl:19-24
+template 
+void test_temp(__global int *par) {
+  T *var1 = addrspace_cast(par);
+  __private T *var2 = addrspace_cast<__private T *>(par);
+  T var3 = addrspace_cast(par);
+}

Anastasia wrote:
> mantognini wrote:
> > Does this test anything since it's not instantiated?
> It tests AST creation and Sema  but not after the TreeTransforms. I have now 
> enhanced it.
It is now much clearer.


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

https://reviews.llvm.org/D60193



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

[PATCH] D124462: [Analyzer] Fix clang::ento::taint::dumpTaint definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, 
xazax.hun.
Herald added a project: All.
mantognini published this revision for review.
mantognini added reviewers: NoQ, xazax.hun.
mantognini added a comment.
Herald added subscribers: cfe-commits, rnkovacs.
Herald added a project: clang.

I'd appreciate reviews for this small patch. Thanks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124462

Files:
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp


Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -37,7 +37,9 @@
 Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+LLVM_DUMP_METHOD void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
 const LocationContext *LCtx,


Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -37,7 +37,9 @@
 Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+LLVM_DUMP_METHOD void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
 const LocationContext *LCtx,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124461: [Analyzer] Remove undefined function

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
mantognini added reviewers: NoQ, baloghadamsoftware.
mantognini published this revision for review.
mantognini added a comment.
Herald added subscribers: cfe-commits, rnkovacs.
Herald added a project: clang.

I'd appreciate reviews for this small patch. Thanks.


This getLValue function was declared in 98db1f990fc2 
 
([Analyzer] [NFC]
Parameter Regions, 2020-05-11) but was never implemented.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124461

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h


Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -306,10 +306,6 @@
   Loc getLValue(const CXXRecordDecl *BaseClass, const SubRegion *Super,
 bool IsVirtual) const;
 
-  /// Get the lvalue for a parameter.
-  Loc getLValue(const Expr *Call, unsigned Index,
-const LocationContext *LC) const;
-
   /// Get the lvalue for a variable reference.
   Loc getLValue(const VarDecl *D, const LocationContext *LC) const;
 


Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -306,10 +306,6 @@
   Loc getLValue(const CXXRecordDecl *BaseClass, const SubRegion *Super,
 bool IsVirtual) const;
 
-  /// Get the lvalue for a parameter.
-  Loc getLValue(const Expr *Call, unsigned Index,
-const LocationContext *LC) const;
-
   /// Get the lvalue for a variable reference.
   Loc getLValue(const VarDecl *D, const LocationContext *LC) const;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124462: [Analyzer] Fix clang::ento::taint::dumpTaint definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D124462#3476581 , @steakhal wrote:

> Could you please elaborate whats the motivation for this change?

Right, I could have been more explicit. Sorry about that.

`dumpTaint` is declared inside the `taint` namespace in the header file, hence 
why I'm adding `taint::`.  I believe `LLVM_DUMP_METHOD` should also be applied 
to the function definition too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124462

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

[PATCH] D124461: [Analyzer] Remove undefined function

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbf0bcb5e539b: [Analyzer] Remove undefined function (authored 
by mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124461

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h


Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -306,10 +306,6 @@
   Loc getLValue(const CXXRecordDecl *BaseClass, const SubRegion *Super,
 bool IsVirtual) const;
 
-  /// Get the lvalue for a parameter.
-  Loc getLValue(const Expr *Call, unsigned Index,
-const LocationContext *LC) const;
-
   /// Get the lvalue for a variable reference.
   Loc getLValue(const VarDecl *D, const LocationContext *LC) const;
 


Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -306,10 +306,6 @@
   Loc getLValue(const CXXRecordDecl *BaseClass, const SubRegion *Super,
 bool IsVirtual) const;
 
-  /// Get the lvalue for a parameter.
-  Loc getLValue(const Expr *Call, unsigned Index,
-const LocationContext *LC) const;
-
   /// Get the lvalue for a variable reference.
   Loc getLValue(const VarDecl *D, const LocationContext *LC) const;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124462: [Analyzer] Fix clang::ento::taint::dumpTaint definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D124462#3477115 , @steakhal wrote:

> Although gcc (and probably clang too) allows specifying 
> `__attribute__((noinline))` at any declaration (by merging //compatible// 
> attributes), I would prefer not to repeat ourselves.
> The attribute must be present at the header to force all usages not to inline 
> it, hence I would rather drop such attributes from the definition files.

On further inspection, I agree with you and will update the patch (+ 
description).

A few things misled me. First, attributes are complex, with standard and 
compiler-specific ones having different requirements. Then, Clang's doc 
 about `used` talks 
about definitions, not declarations. GCC is clearer and always talks about 
declarations. Fortunately, https://godbolt.org/z/PnW7x9Ezz shows Clang 
inherits, as expected, the attributes of interest here.

But I was mainly misled because, specifically for `LLVM_DUMP_METHOD`, the trend 
in LLVM seems to apply it to definitions:

  $ git grep -Ee '::dump\(\) const \{.*' | grep -ve LLVM_DUMP_METHOD | wc -l
  92
  $ git grep -Ee '::dump\(\) const \{.*' | grep -e LLVM_DUMP_METHOD | wc -l
  184

Anyway, thanks for your feedback -- this fairly trivial patch turned out to 
reveal something interesting to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124462

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

[PATCH] D124614: [Analyzer] Fix clang::ento::taint::dumpTaint definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, 
xazax.hun.
Herald added a project: All.
mantognini requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Ensure the definition is in the "taint" namespace, like its declaration.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124614

Files:
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp


Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -37,7 +37,9 @@
 Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
 const LocationContext *LCtx,


Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -37,7 +37,9 @@
 Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
 const LocationContext *LCtx,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124614: [Analyzer] Fix clang::ento::taint::dumpTaint definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini abandoned this revision.
mantognini added a comment.

Ran wrong `arc` command...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124614

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

[PATCH] D124462: [Analyzer] Fix clang::ento::taint::dumpTaint definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 425772.
mantognini added a comment.

Remove LLVM_DUMP_METHOD from definition


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124462

Files:
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp


Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -37,7 +37,9 @@
 Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
 const LocationContext *LCtx,


Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -37,7 +37,9 @@
 Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
 const LocationContext *LCtx,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124621: CPP-2461 [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, 
xazax.hun.
Herald added a project: All.
mantognini added reviewers: dcoughlin, NoQ, r.stahl, xazax.hun.
mantognini published this revision for review.
mantognini added a comment.
Herald added subscribers: cfe-commits, rnkovacs.
Herald added a project: clang.

One thing I'm not sure about and couldn't easily find in the doc is how to 
reference in the commit message the bug (https://llvm.org/PR48534) this patch 
fixes. Is it good as is?


Essentially, having a default member initializer for a constant member
does not necessarily imply the member will have the given default value.

Fix PR48534.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124621

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/cxx-member-initializer-const-field.cpp

Index: clang/test/Analysis/cxx-member-initializer-const-field.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-member-initializer-const-field.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// This tests false-positive issues related to PR48534.
+//
+// Essentially, having a default member initializer for a constant member does
+// not necessarily imply the member will have the given default value.
+
+struct WithConstructor {
+  int *const ptr = nullptr;
+  WithConstructor(int *x) : ptr(x) {}
+
+  static auto compliant() {
+WithConstructor c(new int);
+return *(c.ptr); // no warning
+  }
+
+  static auto compliantWithParam(WithConstructor c) {
+return *(c.ptr); // no warning
+  }
+
+  static auto issue() {
+WithConstructor c(nullptr);
+return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct RegularAggregate {
+  int *const ptr = nullptr;
+
+  static int compliant() {
+RegularAggregate c{new int};
+return *(c.ptr); // no warning
+  }
+
+  static int issue() {
+RegularAggregate c;
+return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct WithConstructorAndArithmetic {
+  int const i = 0;
+  WithConstructorAndArithmetic(int x) : i(x + 1) {}
+
+  static int compliant(int y) {
+WithConstructorAndArithmetic c(0);
+return y / c.i; // no warning
+  }
+
+  static int issue(int y) {
+WithConstructorAndArithmetic c(-1);
+return y / c.i; // expected-warning{{Division by zero}}
+  }
+};
+
+struct WithConstructorDeclarationOnly {
+  int const i = 0;
+  WithConstructorDeclarationOnly(int x); // definition not visible.
+
+  static int compliant1(int y) {
+WithConstructorDeclarationOnly c(0);
+return y / c.i; // no warning
+  }
+
+  static int compliant2(int y) {
+WithConstructorDeclarationOnly c(-1);
+return y / c.i; // no warning
+  }
+};
+
+// NonAggregateFP is not an aggregate (j is a private non-static field) and has no custom constructor.
+// So we know i and j will always be 0 and 42, respectively.
+// That being said, this is not implemented because it is deemed too rare to be worth the complexity.
+struct NonAggregateFP {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+public:
+  static int falsePositive1(NonAggregateFP c) {
+return 10 / c.i; // Currently, no warning.
+  }
+
+  static int falsePositive2(NonAggregateFP c) {
+return 10 / (c.j - 42); // Currently, no warning.
+  }
+};
+
+struct NonAggregate {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+  NonAggregate(NonAggregate const &); // not provided, could set i and j to arbitrary values.
+
+public:
+  static int compliant1(NonAggregate c) {
+return 10 / c.i; // no warning
+  }
+
+  static int compliant2(NonAggregate c) {
+return 10 / (c.j - 42); // no warning
+  }
+};
+
+struct WithStaticMember {
+  static int const i = 0;
+
+  static int issue1(WithStaticMember c) {
+return 10 / c.i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+
+  static int issue2() {
+return 10 / WithStaticMember::i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+};
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1983,15 +1983,9 @@
   if (const Optional &V = B.getDirectBinding(R))
 return *V;
 
-  // Is the field declared constant and has an in-class initializer?
+  // If the containing record was initialized, try to get its constant value.
   const FieldDecl *FD = R->getDecl();
   QualType Ty = FD->getType();
-  if (Ty.isConstQualified())
-if (const Expr *Init = FD->getInClassInitializer())
-  if (Opti

[PATCH] D124462: [Analyzer] Fix clang::ento::taint::dumpTaint definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D124462#3480348 , @aaron.ballman 
wrote:

> In D124462#3480219 , @steakhal 
> wrote:
>
>> Thanks for the stats.
>> @aaron.ballman WDYT, where should we put the `LLVM_DUMP_METHOD` ?
>
> The documentation for the attribute says function definitions, but I don't 
> think it matters in terms of the semantics of the attributes. I'd probably 
> put it on the definition in this case because the goal is to keep the 
> function around at runtime but it doesn't impact the interface of the call or 
> how users would use it at compile time.
>
> I noticed that we seem to be pretty bad about this advice however:
>
>   /// Note that you should also surround dump() functions with
>   /// `#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` so they do always
>   /// get stripped in release builds.

I don't have a strong opinion on where `LLVM_DUMP_METHOD` should be and whether 
this means writing it twice as I can see arguments for both approaches.

In addition to not ifdef-out those functions, I note that many `dump` or 
`dumpToStream` functions in `Analysis` (and LLVM in general) don't have the 
`LLVM_DUMP_METHOD` macro at all.

So I'm tempted to say the overall situation should be improved in a separate 
effort, and this patch can focus only on the orthogonal linking issue. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124462

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

[PATCH] D124621: CPP-2461 [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 426077.
mantognini added a comment.

Update commit message and add FIXMEs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/cxx-member-initializer-const-field.cpp

Index: clang/test/Analysis/cxx-member-initializer-const-field.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-member-initializer-const-field.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// This tests false-positive issues related to PR48534.
+//
+// Essentially, having a default member initializer for a constant member does
+// not necessarily imply the member will have the given default value.
+
+struct WithConstructor {
+  int *const ptr = nullptr;
+  WithConstructor(int *x) : ptr(x) {}
+
+  static auto compliant() {
+WithConstructor c(new int);
+return *(c.ptr); // no warning
+  }
+
+  static auto compliantWithParam(WithConstructor c) {
+return *(c.ptr); // no warning
+  }
+
+  static auto issue() {
+WithConstructor c(nullptr);
+return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct RegularAggregate {
+  int *const ptr = nullptr;
+
+  static int compliant() {
+RegularAggregate c{new int};
+return *(c.ptr); // no warning
+  }
+
+  static int issue() {
+RegularAggregate c;
+return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct WithConstructorAndArithmetic {
+  int const i = 0;
+  WithConstructorAndArithmetic(int x) : i(x + 1) {}
+
+  static int compliant(int y) {
+WithConstructorAndArithmetic c(0);
+return y / c.i; // no warning
+  }
+
+  static int issue(int y) {
+WithConstructorAndArithmetic c(-1);
+return y / c.i; // expected-warning{{Division by zero}}
+  }
+};
+
+struct WithConstructorDeclarationOnly {
+  int const i = 0;
+  WithConstructorDeclarationOnly(int x); // definition not visible.
+
+  static int compliant1(int y) {
+WithConstructorDeclarationOnly c(0);
+return y / c.i; // no warning
+  }
+
+  static int compliant2(int y) {
+WithConstructorDeclarationOnly c(-1);
+return y / c.i; // no warning
+  }
+};
+
+// NonAggregateFP is not an aggregate (j is a private non-static field) and has no custom constructor.
+// So we know i and j will always be 0 and 42, respectively.
+// That being said, this is not implemented because it is deemed too rare to be worth the complexity.
+struct NonAggregateFP {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+public:
+  static int falsePositive1(NonAggregateFP c) {
+return 10 / c.i; // FIXME: Currently, no warning.
+  }
+
+  static int falsePositive2(NonAggregateFP c) {
+return 10 / (c.j - 42); // FIXME: Currently, no warning.
+  }
+};
+
+struct NonAggregate {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+  NonAggregate(NonAggregate const &); // not provided, could set i and j to arbitrary values.
+
+public:
+  static int compliant1(NonAggregate c) {
+return 10 / c.i; // no warning
+  }
+
+  static int compliant2(NonAggregate c) {
+return 10 / (c.j - 42); // no warning
+  }
+};
+
+struct WithStaticMember {
+  static int const i = 0;
+
+  static int issue1(WithStaticMember c) {
+return 10 / c.i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+
+  static int issue2() {
+return 10 / WithStaticMember::i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+};
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1983,15 +1983,9 @@
   if (const Optional &V = B.getDirectBinding(R))
 return *V;
 
-  // Is the field declared constant and has an in-class initializer?
+  // If the containing record was initialized, try to get its constant value.
   const FieldDecl *FD = R->getDecl();
   QualType Ty = FD->getType();
-  if (Ty.isConstQualified())
-if (const Expr *Init = FD->getInClassInitializer())
-  if (Optional V = svalBuilder.getConstantVal(Init))
-return *V;
-
-  // If the containing record was initialized, try to get its constant value.
   const MemRegion* superR = R->getSuperRegion();
   if (const auto *VR = dyn_cast(superR)) {
 const VarDecl *VD = VR->getDecl();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124621: CPP-2461 [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

mantognini marked 2 inline comments as done.
mantognini added a comment.

In D124621#3481973 , @steakhal wrote:

> LGTM
>
> In D124621#3481906 , @mantognini 
> wrote:
>
>> One thing I'm not sure about and couldn't easily find in the doc is how to 
>> reference in the commit message the bug (https://llvm.org/PR48534) this 
>> patch fixes. Is it good as is?
>
> AFAIK we should prefer GitHub issue numbers to the old BugZilla numbers.
> Could you please update all references of `48534` -> `47878`.
>
> In addition, I think `Fixes #47878` should work in the commit message, and 
> automatically close the given GitHub issue.

Thanks for the feedback!

> BTW have you measured the observable impact of this patch on large codebases? 
> Do you have any stats?

I can't share the data but I can say it fixes some user reports. :-)

In D124621#3481981 , @steakhal wrote:

> Noq wrote 
> https://github.com/llvm/llvm-project/issues/47878#issuecomment-981036634
>
>> Aha, uhm, yeah, i see. The static analyzer indeed thinks that a combination 
>> of "const" and a field initializer causes the field to forever stay that 
>> way. **We'll need to undo this relatively recently added shortcut.**
>
> What "recently added shortcut" did he mention? Could you please refer to that 
> commit in the patch summary, please?

I think @NoQ refers to https://reviews.llvm.org/D45774 but I'll wait for a week 
or so for confirmation in case there's more to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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

[PATCH] D124681: [Analyzer] Minor cleanups in StreamChecker

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, 
xazax.hun.
Herald added a project: All.
mantognini added reviewers: NoQ, balazske, Szelethus.
mantognini published this revision for review.
mantognini added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For reference, the modified code was introduced with 
https://reviews.llvm.org/D80015.

Any feedback is appreciated.


Remove unnecessary conversion to Optional<> and incorrect assumption
that BindExpr can return a null state.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124681

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -672,24 +672,19 @@
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
 ProgramStateRef StateNotFailed =
 State->BindExpr(CE, C.getLocationContext(), *NMembVal);
-if (StateNotFailed) {
-  StateNotFailed = StateNotFailed->set(
-  StreamSym, StreamState::getOpened(Desc));
-  C.addTransition(StateNotFailed);
-}
+StateNotFailed =
+StateNotFailed->set(StreamSym, 
StreamState::getOpened(Desc));
+C.addTransition(StateNotFailed);
   }
 
   // Add transition for the failed state.
-  Optional RetVal = makeRetVal(C, CE).castAs();
-  assert(RetVal && "Value should be NonLoc.");
+  NonLoc RetVal = makeRetVal(C, CE).castAs();
   ProgramStateRef StateFailed =
-  State->BindExpr(CE, C.getLocationContext(), *RetVal);
-  if (!StateFailed)
-return;
-  auto Cond = C.getSValBuilder()
-  .evalBinOpNN(State, BO_LT, *RetVal, *NMembVal,
-   C.getASTContext().IntTy)
-  .getAs();
+  State->BindExpr(CE, C.getLocationContext(), RetVal);
+  auto Cond =
+  C.getSValBuilder()
+  .evalBinOpNN(State, BO_LT, RetVal, *NMembVal, 
C.getASTContext().IntTy)
+  .getAs();
   if (!Cond)
 return;
   StateFailed = StateFailed->assume(*Cond, true);


Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -672,24 +672,19 @@
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
 ProgramStateRef StateNotFailed =
 State->BindExpr(CE, C.getLocationContext(), *NMembVal);
-if (StateNotFailed) {
-  StateNotFailed = StateNotFailed->set(
-  StreamSym, StreamState::getOpened(Desc));
-  C.addTransition(StateNotFailed);
-}
+StateNotFailed =
+StateNotFailed->set(StreamSym, StreamState::getOpened(Desc));
+C.addTransition(StateNotFailed);
   }
 
   // Add transition for the failed state.
-  Optional RetVal = makeRetVal(C, CE).castAs();
-  assert(RetVal && "Value should be NonLoc.");
+  NonLoc RetVal = makeRetVal(C, CE).castAs();
   ProgramStateRef StateFailed =
-  State->BindExpr(CE, C.getLocationContext(), *RetVal);
-  if (!StateFailed)
-return;
-  auto Cond = C.getSValBuilder()
-  .evalBinOpNN(State, BO_LT, *RetVal, *NMembVal,
-   C.getASTContext().IntTy)
-  .getAs();
+  State->BindExpr(CE, C.getLocationContext(), RetVal);
+  auto Cond =
+  C.getSValBuilder()
+  .evalBinOpNN(State, BO_LT, RetVal, *NMembVal, C.getASTContext().IntTy)
+  .getAs();
   if (!Cond)
 return;
   StateFailed = StateFailed->assume(*Cond, true);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D124621#3482847 , @steakhal wrote:

> For the upcoming patches, it would be nice to test the patches on a small set 
> of open-source projects for exactly this reason.
> I think there is a `clang/utils/analyzer/SATest.py` script helping you on 
> this part.
> It seems we have quite a few projects on the testset 
> `clang/utils/analyzer/projects/projects.json`.
> We are not using it, because we have a different internal testing 
> infrastructure, but it's definitely better than nothing.

Thanks for the tip. I had to fix a thing or two to get SATest.py working with 
my setup (I'll try to upstream those fixes at some point). However, these 
projects do not highlight the false-positive being fixed here. I.e., I get no 
difference in reports with and without this patch. But I'll keep this in mind 
when working on other improvements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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

[PATCH] D124462: [Analyzer] Fix clang::ento::taint::dumpTaint definition

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5a47accda88c: [Analyzer] Fix clang::ento::taint::dumpTaint 
definition (authored by mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124462

Files:
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp


Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -37,7 +37,9 @@
 Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
 const LocationContext *LCtx,


Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -37,7 +37,9 @@
 Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
 const LocationContext *LCtx,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124681: [Analyzer] Minor cleanups in StreamChecker

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf34639828f5a: [Analyzer] Minor cleanups in StreamChecker 
(authored by mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124681

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -672,24 +672,19 @@
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
 ProgramStateRef StateNotFailed =
 State->BindExpr(CE, C.getLocationContext(), *NMembVal);
-if (StateNotFailed) {
-  StateNotFailed = StateNotFailed->set(
-  StreamSym, StreamState::getOpened(Desc));
-  C.addTransition(StateNotFailed);
-}
+StateNotFailed =
+StateNotFailed->set(StreamSym, 
StreamState::getOpened(Desc));
+C.addTransition(StateNotFailed);
   }
 
   // Add transition for the failed state.
-  Optional RetVal = makeRetVal(C, CE).castAs();
-  assert(RetVal && "Value should be NonLoc.");
+  NonLoc RetVal = makeRetVal(C, CE).castAs();
   ProgramStateRef StateFailed =
-  State->BindExpr(CE, C.getLocationContext(), *RetVal);
-  if (!StateFailed)
-return;
-  auto Cond = C.getSValBuilder()
-  .evalBinOpNN(State, BO_LT, *RetVal, *NMembVal,
-   C.getASTContext().IntTy)
-  .getAs();
+  State->BindExpr(CE, C.getLocationContext(), RetVal);
+  auto Cond =
+  C.getSValBuilder()
+  .evalBinOpNN(State, BO_LT, RetVal, *NMembVal, 
C.getASTContext().IntTy)
+  .getAs();
   if (!Cond)
 return;
   StateFailed = StateFailed->assume(*Cond, true);


Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -672,24 +672,19 @@
   if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
 ProgramStateRef StateNotFailed =
 State->BindExpr(CE, C.getLocationContext(), *NMembVal);
-if (StateNotFailed) {
-  StateNotFailed = StateNotFailed->set(
-  StreamSym, StreamState::getOpened(Desc));
-  C.addTransition(StateNotFailed);
-}
+StateNotFailed =
+StateNotFailed->set(StreamSym, StreamState::getOpened(Desc));
+C.addTransition(StateNotFailed);
   }
 
   // Add transition for the failed state.
-  Optional RetVal = makeRetVal(C, CE).castAs();
-  assert(RetVal && "Value should be NonLoc.");
+  NonLoc RetVal = makeRetVal(C, CE).castAs();
   ProgramStateRef StateFailed =
-  State->BindExpr(CE, C.getLocationContext(), *RetVal);
-  if (!StateFailed)
-return;
-  auto Cond = C.getSValBuilder()
-  .evalBinOpNN(State, BO_LT, *RetVal, *NMembVal,
-   C.getASTContext().IntTy)
-  .getAs();
+  State->BindExpr(CE, C.getLocationContext(), RetVal);
+  auto Cond =
+  C.getSValBuilder()
+  .evalBinOpNN(State, BO_LT, RetVal, *NMembVal, C.getASTContext().IntTy)
+  .getAs();
   if (!Cond)
 return;
   StateFailed = StateFailed->assume(*Cond, true);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D124621#3486004 , @r.stahl wrote:

> I can confirm the issue with my patch, so this piece of code needs to be 
> removed.
>
> As long as the following test still succeeds, this looks good to me. Back 
> then, the analyzer was not able to cover that case without that addition.
>
> https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/globals.cpp#L110

Thanks for digging in the past. I confirm I didn't see any regression in 
existing tests, so I'll land it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

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

[PATCH] D124621: [Analyzer] Fix assumptions about const field with member-initializer

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG68ee5ec07d4a: [Analyzer] Fix assumptions about const field 
with member-initializer (authored by mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124621

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/cxx-member-initializer-const-field.cpp

Index: clang/test/Analysis/cxx-member-initializer-const-field.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-member-initializer-const-field.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// This tests false-positive issues related to PR48534.
+//
+// Essentially, having a default member initializer for a constant member does
+// not necessarily imply the member will have the given default value.
+
+struct WithConstructor {
+  int *const ptr = nullptr;
+  WithConstructor(int *x) : ptr(x) {}
+
+  static auto compliant() {
+WithConstructor c(new int);
+return *(c.ptr); // no warning
+  }
+
+  static auto compliantWithParam(WithConstructor c) {
+return *(c.ptr); // no warning
+  }
+
+  static auto issue() {
+WithConstructor c(nullptr);
+return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct RegularAggregate {
+  int *const ptr = nullptr;
+
+  static int compliant() {
+RegularAggregate c{new int};
+return *(c.ptr); // no warning
+  }
+
+  static int issue() {
+RegularAggregate c;
+return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
+  }
+};
+
+struct WithConstructorAndArithmetic {
+  int const i = 0;
+  WithConstructorAndArithmetic(int x) : i(x + 1) {}
+
+  static int compliant(int y) {
+WithConstructorAndArithmetic c(0);
+return y / c.i; // no warning
+  }
+
+  static int issue(int y) {
+WithConstructorAndArithmetic c(-1);
+return y / c.i; // expected-warning{{Division by zero}}
+  }
+};
+
+struct WithConstructorDeclarationOnly {
+  int const i = 0;
+  WithConstructorDeclarationOnly(int x); // definition not visible.
+
+  static int compliant1(int y) {
+WithConstructorDeclarationOnly c(0);
+return y / c.i; // no warning
+  }
+
+  static int compliant2(int y) {
+WithConstructorDeclarationOnly c(-1);
+return y / c.i; // no warning
+  }
+};
+
+// NonAggregateFP is not an aggregate (j is a private non-static field) and has no custom constructor.
+// So we know i and j will always be 0 and 42, respectively.
+// That being said, this is not implemented because it is deemed too rare to be worth the complexity.
+struct NonAggregateFP {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+public:
+  static int falsePositive1(NonAggregateFP c) {
+return 10 / c.i; // FIXME: Currently, no warning.
+  }
+
+  static int falsePositive2(NonAggregateFP c) {
+return 10 / (c.j - 42); // FIXME: Currently, no warning.
+  }
+};
+
+struct NonAggregate {
+public:
+  int const i = 0;
+
+private:
+  int const j = 42;
+
+  NonAggregate(NonAggregate const &); // not provided, could set i and j to arbitrary values.
+
+public:
+  static int compliant1(NonAggregate c) {
+return 10 / c.i; // no warning
+  }
+
+  static int compliant2(NonAggregate c) {
+return 10 / (c.j - 42); // no warning
+  }
+};
+
+struct WithStaticMember {
+  static int const i = 0;
+
+  static int issue1(WithStaticMember c) {
+return 10 / c.i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+
+  static int issue2() {
+return 10 / WithStaticMember::i; // expected-warning{{division by zero is undefined}} expected-warning{{Division by zero}}
+  }
+};
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1983,15 +1983,9 @@
   if (const Optional &V = B.getDirectBinding(R))
 return *V;
 
-  // Is the field declared constant and has an in-class initializer?
+  // If the containing record was initialized, try to get its constant value.
   const FieldDecl *FD = R->getDecl();
   QualType Ty = FD->getType();
-  if (Ty.isConstQualified())
-if (const Expr *Init = FD->getInClassInitializer())
-  if (Optional V = svalBuilder.getConstantVal(Init))
-return *V;
-
-  // If the containing record was initialized, try to get its constant value.
   const MemRegion* superR = R->getSuperRegion();
   if (const auto *VR = dyn_cast(superR)) {
 const VarDecl *VD = VR->getDecl();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGad47114ad850: In MSVC compatibility mode, friend function 
declarations behave as function… (authored by frederic-tingaud-sonarsource, 
committed by mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124613

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/ms-friend-function-decl.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -2658,7 +2658,10 @@
   getTuDecl("struct X { friend void f(); };", Lang_CXX03, "input0.cc");
   auto *FromD = FirstDeclMatcher().match(FromTU, FunctionPattern);
   ASSERT_TRUE(FromD->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromTU->getLangOpts().MSVCCompat,
+FromD->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   {
 auto FromName = FromD->getDeclName();
 auto *Class = FirstDeclMatcher().match(FromTU, ClassPattern);
@@ -2702,7 +2705,10 @@
   auto *FromNormal =
   LastDeclMatcher().match(FromTU, FunctionPattern);
   ASSERT_TRUE(FromFriend->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromTU->getLangOpts().MSVCCompat,
+FromFriend->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_FALSE(FromNormal->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   ASSERT_TRUE(FromNormal->isInIdentifierNamespace(Decl::IDNS_Ordinary));
 
@@ -2793,7 +2799,10 @@
 
   ASSERT_TRUE(FromNormalF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_FALSE(FromNormalF->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
-  ASSERT_FALSE(FromFriendF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
+  // Before CXX20, MSVC treats friend function declarations as function
+  // declarations
+  ASSERT_EQ(FromFriendTU->getLangOpts().MSVCCompat,
+FromFriendF->isInIdentifierNamespace(Decl::IDNS_Ordinary));
   ASSERT_TRUE(FromFriendF->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend));
   auto LookupRes = FromNormalTU->noload_lookup(FromNormalName);
   ASSERT_TRUE(LookupRes.isSingleResult());
Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern %s
+#if __cplusplus < 202002L
+// expected-no-diagnostics
+#endif
+
+namespace ns {
+
+class C {
+public:
+  template 
+  friend void funtemp();
+
+  friend void fun();
+
+  void test() {
+::ns::fun(); // modern-error {{no member named 'fun' in namespace 'ns'}}
+
+// modern-error@+3 {{no member named 'funtemp' in namespace 'ns'}}
+// modern-error@+2 {{expected '(' for function-style cast or type construction}}
+// modern-error@+1 {{expected expression}}
+::ns::funtemp();
+  }
+};
+
+void fun() {
+}
+
+template 
+void funtemp() {}
+
+} // namespace ns
+
+class Glob {
+public:
+  friend void funGlob();
+
+  void test() {
+funGlob(); // modern-error {{use of undeclared identifier 'funGlob'}}
+  }
+};
+
+void funGlob() {
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -9632,11 +9632,15 @@
 }
 
 if (isFriend) {
+  // In MSVC mode for older versions of the standard, friend function
+  // declarations behave as declarations
+  bool PerformFriendInjection =
+  getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20;
   if (FunctionTemplate) {
-FunctionTemplate->setObjectOfFriendDecl();
+FunctionTemplate->setObjectOfFriendDecl(PerformFriendInjection);
 FunctionTemplate->setAccess(AS_public);
   }
-  NewFD->setObjectOfFriendDecl();
+  NewFD->setObjectOfFriendDecl(PerformFriendInjection);
   NewFD->setAccess(AS_public);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

[Marco Antognini via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc894e85fc64d: In MSVC compatibility mode, handle unqualified 
templated base class… (authored by frederic-tingaud-sonarsource, committed by 
mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaTemplate/ms-unqualified-base-class.cpp

Index: clang/test/SemaTemplate/ms-unqualified-base-class.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/ms-unqualified-base-class.cpp
@@ -0,0 +1,85 @@
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=before,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=after,expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fdelayed-template-parsing -fsyntax-only -verify=after,expected %s
+
+template 
+class Base {
+};
+
+template 
+class Based {}; // Trying to trick the typo detection
+
+template 
+class Derived : public Base {
+public:
+  // after-error@+1 {{member initializer 'Base' does not name a non-static data member or base class}}
+  Derived() : Base() {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+private:
+  int Baze; // Trying to trick the typo detection
+};
+
+template  struct AggregateBase {
+  T i;
+};
+
+template 
+struct AggregateDerived : public AggregateBase {
+  int i;
+
+  // after-error@+1 {{member initializer 'AggregateBase' does not name a non-static data member or base class}}
+  AggregateDerived(T j) : AggregateBase{4}, i{j} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+  int f() {
+return i + AggregateBase::i; // expected-warning {{use of undeclared identifier 'AggregateBase'; unqualified lookup into dependent bases of class template 'AggregateDerived' is a Microsoft extension}}
+  }
+};
+
+template  struct MultiTypesBase {
+};
+
+template 
+struct MultiTypesDerived : public MultiTypesBase {
+  // after-error@+1 {{member initializer 'MultiTypesBase' does not name a non-static data member or base class}}
+  MultiTypesDerived() : MultiTypesBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct IntegerBase {
+};
+
+template 
+struct IntegerDerived : public IntegerBase {
+  // after-error@+1 {{member initializer 'IntegerBase' does not name a non-static data member or base class}}
+  IntegerDerived() : IntegerBase{} {} // before-warning {{unqualified base initializer of class templates is a Microsoft extension}}
+};
+
+template  struct ConformingBase {
+  T i;
+};
+
+template 
+struct ConformingDerived : public ConformingBase {
+  int i;
+
+  ConformingDerived(T j) : ConformingBase{4}, i{j} {}
+  int f() {
+return i + ConformingBase::i;
+  }
+};
+
+int main() {
+  int I;
+  Derived t;
+
+  AggregateDerived AD{2};
+  AD.AggregateBase::i = 3;
+  I = AD.f();
+
+  MultiTypesDerived MTD;
+
+  IntegerDerived<4> ID;
+
+  ConformingDerived CD{2};
+  I = CD.f();
+
+  return I;
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -4307,6 +4307,15 @@
 }
   }
 
+  if (getLangOpts().MSVCCompat && !getLangOpts().CPlusPlus20) {
+auto UnqualifiedBase = R.getAsSingle();
+if (UnqualifiedBase) {
+  Diag(IdLoc, diag::ext_unqualified_base_class)
+  << SourceRange(IdLoc, Init->getSourceRange().getEnd());
+  BaseType = UnqualifiedBase->getInjectedClassNameSpecialization();
+}
+  }
+
   // If no results were found, try to correct typos.
   TypoCorrection Corr;
   MemInitializerValidatorCCC CCC(ClassDecl);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5509,6 +5509,9 @@
 def ext_found_later_in_class : ExtWarn<
   "use of member %0 before its declaration is a Microsoft extension">,
   InGroup;
+def ext_unqualified_base_class : ExtWarn<
+  "unqualified base initializer of class templates is a Microsoft extension">,
+  InGroup;
 def note_dependent_member_use : Note<
   "must qualify identifier to find this declaration in dependent base class">;
 def err_not_found_by_two_phase_lookup : Error<"call to function %0 that is neither "
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D126196: [analyzer] SATest: Ensure Docker image can be built

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
mantognini edited the summary of this revision.
mantognini published this revision for review.
mantognini added reviewers: vsavchenko, steakhal.
mantognini added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch relates to my previous comment: 
https://reviews.llvm.org/D124621#3485799


Solve build issues occurring when running `docker build`.

Fix the version of cmake-data to solve the following issue:

  The following packages have unmet dependencies:
   cmake : Depends: cmake-data (= 3.20.5-0kitware1) but 
3.23.1-0kitware1ubuntu18.04.1 is to be installed

Install libjpeg to solve this issue when installing Python
requirements:

  The headers or library files could not be found for jpeg,
  a required dependency when compiling Pillow from source.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126196

Files:
  clang/utils/analyzer/Dockerfile


Index: clang/utils/analyzer/Dockerfile
===
--- clang/utils/analyzer/Dockerfile
+++ clang/utils/analyzer/Dockerfile
@@ -18,6 +18,7 @@
 python3=3.6.7-1~18.04 \
 python3-pip=9.0.1-2.3* \
 cmake=3.20.5* \
+cmake-data=3.20.5* \
 ninja-build=1.8.2-1
 
 # box2d dependencies
@@ -52,6 +53,9 @@
 flex=2.6.4-6 \
 bison=2:3.0.4.*
 
+RUN apt-get install -y \
+libjpeg-dev
+
 RUN update-alternatives --install /usr/bin/python python /usr/bin/python3 1
 
 VOLUME /analyzer


Index: clang/utils/analyzer/Dockerfile
===
--- clang/utils/analyzer/Dockerfile
+++ clang/utils/analyzer/Dockerfile
@@ -18,6 +18,7 @@
 python3=3.6.7-1~18.04 \
 python3-pip=9.0.1-2.3* \
 cmake=3.20.5* \
+cmake-data=3.20.5* \
 ninja-build=1.8.2-1
 
 # box2d dependencies
@@ -52,6 +53,9 @@
 flex=2.6.4-6 \
 bison=2:3.0.4.*
 
+RUN apt-get install -y \
+libjpeg-dev
+
 RUN update-alternatives --install /usr/bin/python python /usr/bin/python3 1
 
 VOLUME /analyzer
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126197: [analyzer] SATest: Weaken assumption about HTML files

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
mantognini published this revision for review.
mantognini added reviewers: vsavchenko, steakhal.
mantognini added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch relates to my previous comment: 
https://reviews.llvm.org/D124621#3485799


Instead of assuming there is an HTML file for each diagnostics, consider
the HTML files only when they exist, individually of each other.

After generating the reference data, running

  python /scripts/SATest.py build --projects simbody

was resulting in this error:

File "/scripts/CmpRuns.py", line 250, in read_single_file
  assert len(d['HTMLDiagnostics_files']) == 1
  KeyError: 'HTMLDiagnostics_files'


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126197

Files:
  clang/utils/analyzer/CmpRuns.py


Index: clang/utils/analyzer/CmpRuns.py
===
--- clang/utils/analyzer/CmpRuns.py
+++ clang/utils/analyzer/CmpRuns.py
@@ -242,17 +242,20 @@
 return
 
 # Extract the HTML reports, if they exists.
-if 'HTMLDiagnostics_files' in data['diagnostics'][0]:
-htmlFiles = []
-for d in data['diagnostics']:
+htmlFiles = []
+for d in data['diagnostics']:
+if 'HTMLDiagnostics_files' in d:
 # FIXME: Why is this named files, when does it have multiple
 # files?
 assert len(d['HTMLDiagnostics_files']) == 1
 htmlFiles.append(d.pop('HTMLDiagnostics_files')[0])
-else:
-htmlFiles = [None] * len(data['diagnostics'])
+else:
+htmlFiles.append(None)
 
 report = AnalysisReport(self, data.pop('files'))
+# Python 3.10 offers zip(..., strict=True). The following assertion
+# mimics it.
+assert len(data['diagnostics']) == len(htmlFiles)
 diagnostics = [AnalysisDiagnostic(d, report, h)
for d, h in zip(data.pop('diagnostics'), htmlFiles)]
 


Index: clang/utils/analyzer/CmpRuns.py
===
--- clang/utils/analyzer/CmpRuns.py
+++ clang/utils/analyzer/CmpRuns.py
@@ -242,17 +242,20 @@
 return
 
 # Extract the HTML reports, if they exists.
-if 'HTMLDiagnostics_files' in data['diagnostics'][0]:
-htmlFiles = []
-for d in data['diagnostics']:
+htmlFiles = []
+for d in data['diagnostics']:
+if 'HTMLDiagnostics_files' in d:
 # FIXME: Why is this named files, when does it have multiple
 # files?
 assert len(d['HTMLDiagnostics_files']) == 1
 htmlFiles.append(d.pop('HTMLDiagnostics_files')[0])
-else:
-htmlFiles = [None] * len(data['diagnostics'])
+else:
+htmlFiles.append(None)
 
 report = AnalysisReport(self, data.pop('files'))
+# Python 3.10 offers zip(..., strict=True). The following assertion
+# mimics it.
+assert len(data['diagnostics']) == len(htmlFiles)
 diagnostics = [AnalysisDiagnostic(d, report, h)
for d, h in zip(data.pop('diagnostics'), htmlFiles)]
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126197: [analyzer] SATest: Weaken assumption about HTML files

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D126197#3533674 , @steakhal wrote:

> I'm not using this script file, so I'm not gonna be too picky about this.
> However, I would be more confident landing changes if we had tests exercising 
> the changed behavior.
> If you plan to land more changes to this file, consider adding some tests.

I agree tests would be good, but I'm not sure where to start. To clarify, that 
is also the only change I've got to this file and I don't plan further change 
in the near future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126197

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

[PATCH] D126196: [analyzer] SATest: Ensure Docker image can be built

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D126196#3533678 , @steakhal wrote:

> LGTM;
> So you have used this tool. Could you add some notes somewhere on how to get 
> this to work?
> What is the workflow?
> Is it documented anywhere?
>
> I know that's an unrelated topic, but I might consider using/integrating this 
> stuff into our workflow.

Thanks for the review.

It's kind of underdocumented at the moment. Happy to share here the commands 
I've run. I'm not planning to update a README file at this point as I believe 
`--help` messages from various scripts should be clarified too to avoid 
conflicting/confusing information. Unfortunately, I don't have the bandwidth at 
the moment to do that properly.

Essentially, I've run this to 1) build the docker image, 2) create some docker 
volume for persistent storage, and 3) open a shell in a docker container.

  python3 ./clang/utils/analyzer/SATest.py docker --build-image
  docker volume create --name satest-build
  docker volume create --name satest-install
  python3 ./clang/utils/analyzer/SATest.py docker \
--build-dir satest-build --clang-dir satest-install --llvm-project-dir 
$(pwd) \
--shell

Then, in that container, I've run essentially this to generate the reference 
data (mind the final `-r`):

  python /entrypoint.py --build-llvm-only
  cd /projects
  python /scripts/SATest.py build [--projects cxxopts,...] [-j 10] -v -r

Then, to test some changes, I check out the relevant patch and re-run the above 
without `-r` to build and compare the results.

(I guess it should be possible to do all this without manually running commands 
in the container shell, but I haven't figured out how.)

I hope this helps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126196

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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

I think this looks good. Maybe the tests should be extended to test `auto` as 
function return type, and if there's some special handling around 
`decltype(auto)`, then it should be tested too, but I'm not sure it's actually 
needed here. What do you think?




Comment at: lib/Sema/SemaType.cpp:7441
+  // the initializing expression type during the type deduction.
+  (T->isAutoType() && IsPointee) || (IsAutoPointee) ||
   // OpenCL spec v2.0 s6.9.b:

Shouldn't the parentheses around `IsAutoPointee` be removed for style 
consistency?



Comment at: lib/Sema/SemaType.cpp:7441
+  // the initializing expression type during the type deduction.
+  (T->isAutoType() && IsPointee) || (IsAutoPointee) ||
   // OpenCL spec v2.0 s6.9.b:

mantognini wrote:
> Shouldn't the parentheses around `IsAutoPointee` be removed for style 
> consistency?
With the `if` statement introduced above, `IsAutoPointee` can be true only in 
C++ mode. Could it be an issue to not guard `(T->isAutoType() && IsPointee)` 
for non-C++ mode? (I guess not, but I couldn't convince myself.)



Comment at: lib/Sema/TreeTransform.h:4550
+Pointee = Pointee->getPointeeType();
+  }  while (!Pointee.isNull());
+  if (!IsAuto && PointeeType.getAddressSpace() == LangAS::Default)

Nitpicking: there are two spaces between `}` and `while`.


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

https://reviews.llvm.org/D65744



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

[PATCH] D60193: [OpenCL] Added addrspace_cast operator

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.



Comment at: clang/include/clang-c/Index.h:2057
+   */
+  CXCursor_CXXAddrspaceCastExpr = 129,
+

hans wrote:
> Anastasia wrote:
> > hans wrote:
> > > akyrtzi wrote:
> > > > Hi Anastasia, apologies for not catching this earlier, but libclang is 
> > > > intended to keep a stable ABI and changing the enumerator values breaks 
> > > > libclang's ABI compatibility.
> > > > 
> > > > Would you be able to make a follow-up commit and restore the enumerator 
> > > > values to their original values? I would suggest to add 
> > > > `CXCursor_CXXAddrspaceCastExpr` at the end and assign to it the next 
> > > > available value that is not already taken.
> > > It also broke the Python bindings, as someone reported here: 
> > > https://stackoverflow.com/questions/64542827/there-appears-to-be-mismatch-in-clang-llvm-ver-11-0-0-python-bindings
> > I guess it's too late to fix 11.0.0, does it make sense to fix 11.0.1?
> That's @tstellar 's call.
@tstellar, does this require any particular action from my side to be included 
in 11.0.1?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60193

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

[PATCH] D91429: [OpenCL] Stop opencl-c-base.h leaking extension enabling

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

LGMT as it reduces the divergence in compilation flows. I let @Anastasia, or 
someone else more familiar with the codebase, give the final approval though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91429

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

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
Herald added subscribers: cfe-commits, kerbowa, Anastasia, yaxunl, nhaehnle, 
jvesely, jholewinski.
Herald added a project: clang.
mantognini updated this revision to Diff 298093.
mantognini added a comment.
mantognini published this revision for review.

Addressed buildbot issues.


Many non-language extensions are defined but also unused. This patch
removes them with their tests as they do not require compiler support.

The cl_khr_select_fprounding_mode extension is also removed because it
has been deprecated since OpenCL 1.1 and Clang doesn't have any specific
support for it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89372

Files:
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl
  clang/test/Misc/nvptx.languageOptsOpenCL.cl
  clang/test/Misc/r600.languageOptsOpenCL.cl
  clang/test/Preprocessor/init.c
  clang/test/SemaOpenCL/extension-version.cl

Index: clang/test/SemaOpenCL/extension-version.cl
===
--- clang/test/SemaOpenCL/extension-version.cl
+++ clang/test/SemaOpenCL/extension-version.cl
@@ -34,26 +34,8 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifndef cl_khr_gl_sharing
-#error "Missing cl_khr_gl_sharing define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-
-#ifndef cl_khr_icd
-#error "Missing cl_khr_icd define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_icd: enable
-
 // Core features in CL 1.1
 
-#ifndef cl_khr_byte_addressable_store
-#error "Missing cl_khr_byte_addressable_store define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_byte_addressable_store : enable
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110) && defined TEST_CORE_FEATURES
-// expected-warning@-2{{OpenCL extension 'cl_khr_byte_addressable_store' is core feature or supported optional core feature - ignoring}}
-#endif
-
 #ifndef cl_khr_global_int32_base_atomics
 #error "Missing cl_khr_global_int32_base_atomics define"
 #endif
@@ -86,15 +68,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_local_int32_extended_atomics' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 110)
-// Deprecated abvoe 1.0
-#ifndef cl_khr_select_fprounding_mode
-#error "Missing cl_khr_select_fp_rounding_mode define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_select_fprounding_mode: enable
-#endif
-
-
 // Core feature in CL 1.2
 #ifndef cl_khr_fp64
 #error "Missing cl_khr_fp64 define"
@@ -113,87 +86,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_3d_image_writes' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_gl_event
-#error "Missing cl_khr_gl_event define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_event' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_event : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_d3d10_sharing
-#error "Missing cl_khr_d3d10_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d10_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d10_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cles_khr_int64
-#error "Missing cles_khr_int64 define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cles_khr_int64' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cles_khr_int64 : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_context_abort
-#error "Missing cl_context_abort define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_context_abort' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_context_abort : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_d3d11_sharing
-#error "Missing cl_khr_d3d11_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d11_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d11_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_dx9_media_sharing
-#error "Missing cl_khr_dx9_media_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_dx9_media_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_dx9_media_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_image2d_from_buffer
-#error "Missing cl_khr_image2d_from_buffer define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_image2d_from_buffer' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D89372#2329939 , @yaxunl wrote:

> what if users rely on the predefined macros associated with the extension 
> e.g. cl_khr_srgb_image_writes to enable/disable certain code?
>
> What's the issue with these extensions not removed?

I meant to add a link to the RFC that highlighted the issue: 
http://lists.llvm.org/pipermail/cfe-dev/2020-September/066911.html

In a nutshell, those extensions I'm removing are not language extensions but 
api extensions. I can't think of the usefulness of these macros -- if the host 
doesn't support the extensions, the kernels relying on those cannot be executed 
(i.e. it makes no sense). @Anastasia also highlights in her comment that having 
these increases the complexity and maintenance burden in the ecosystem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

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

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:74
 OPENCLEXT_INTERNAL(cl_khr_mipmap_image_writes, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)

azabaznov wrote:
> cl_khr_srgb_image_writes - Extension allowing writes to sRGB images from a 
> kernel. This extension enables write_imagef built-in function as it described 
> [[ 
> https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_srgb_image_writes
>  | here]]. So I think we shouldn't remove it. Do I miss something?
On second reading, this one is slightly ambiguous. On the language side, the 
extension doesn't add an overload; it only specifies that existing overload can 
be used with a different kind of image. On the API side, it does extend the set 
of features. But at the same time if the extended API is not supported, it's 
not possible to create an image in the first place and therefore impossible to 
call write_imagef. So I question the usefulness of such macro on the device 
side. Does this argument convince you it should be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

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

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 298333.
mantognini added a comment.

Addressed comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

Files:
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl
  clang/test/Misc/nvptx.languageOptsOpenCL.cl
  clang/test/Misc/r600.languageOptsOpenCL.cl
  clang/test/Preprocessor/init.c
  clang/test/SemaOpenCL/extension-version.cl

Index: clang/test/SemaOpenCL/extension-version.cl
===
--- clang/test/SemaOpenCL/extension-version.cl
+++ clang/test/SemaOpenCL/extension-version.cl
@@ -34,26 +34,8 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifndef cl_khr_gl_sharing
-#error "Missing cl_khr_gl_sharing define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-
-#ifndef cl_khr_icd
-#error "Missing cl_khr_icd define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_icd: enable
-
 // Core features in CL 1.1
 
-#ifndef cl_khr_byte_addressable_store
-#error "Missing cl_khr_byte_addressable_store define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_byte_addressable_store : enable
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110) && defined TEST_CORE_FEATURES
-// expected-warning@-2{{OpenCL extension 'cl_khr_byte_addressable_store' is core feature or supported optional core feature - ignoring}}
-#endif
-
 #ifndef cl_khr_global_int32_base_atomics
 #error "Missing cl_khr_global_int32_base_atomics define"
 #endif
@@ -86,15 +68,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_local_int32_extended_atomics' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 110)
-// Deprecated abvoe 1.0
-#ifndef cl_khr_select_fprounding_mode
-#error "Missing cl_khr_select_fp_rounding_mode define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_select_fprounding_mode: enable
-#endif
-
-
 // Core feature in CL 1.2
 #ifndef cl_khr_fp64
 #error "Missing cl_khr_fp64 define"
@@ -113,87 +86,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_3d_image_writes' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_gl_event
-#error "Missing cl_khr_gl_event define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_event' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_event : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_d3d10_sharing
-#error "Missing cl_khr_d3d10_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d10_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d10_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cles_khr_int64
-#error "Missing cles_khr_int64 define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cles_khr_int64' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cles_khr_int64 : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_context_abort
-#error "Missing cl_context_abort define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_context_abort' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_context_abort : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_d3d11_sharing
-#error "Missing cl_khr_d3d11_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d11_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d11_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_dx9_media_sharing
-#error "Missing cl_khr_dx9_media_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_dx9_media_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_dx9_media_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_image2d_from_buffer
-#error "Missing cl_khr_image2d_from_buffer define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_image2d_from_buffer' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_image2d_from_buffer : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_initialize_memory
-#error "Missing cl_khr_initialize_memory define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_initialize_memory' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_initialize_memory : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_kh

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added inline comments.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:16
 //
 // If the extensions are to be enumerated without the supported OpenCL version,
 // define OPENCLEXT(ext) where ext is the name of the extension.

Anastasia wrote:
> yaxunl wrote:
> > Can you add a comment here referring the spec about "Every extension which 
> > affects the OpenCL language semantics, syntax or adds built-in functions to 
> > the language must create a preprocessor #define that matches the extension 
> > name string." and therefore only extensions "which affects the OpenCL 
> > language semantics, syntax or adds built-in functions to the language" are 
> > defined in this file. Thanks.
> Good idea! I also suggest we add clarifications regarding pragmas, that those 
> are to be added only when the compiler needs to change the compilation flow 
> i.e. if there is a difference in the language semantic from what is defined 
> in a standard.
I've added a comment. Let me know if it suits you.



Comment at: clang/include/clang/Basic/OpenCLExtensions.def:74
 OPENCLEXT_INTERNAL(cl_khr_mipmap_image_writes, 200, ~0U)
-OPENCLEXT_INTERNAL(cl_khr_srgb_image_writes, 200, ~0U)
 OPENCLEXT_INTERNAL(cl_khr_subgroups, 200, ~0U)

Anastasia wrote:
> azabaznov wrote:
> > mantognini wrote:
> > > azabaznov wrote:
> > > > cl_khr_srgb_image_writes - Extension allowing writes to sRGB images 
> > > > from a kernel. This extension enables write_imagef built-in function as 
> > > > it described [[ 
> > > > https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Ext.html#cl_khr_srgb_image_writes
> > > >  | here]]. So I think we shouldn't remove it. Do I miss something?
> > > On second reading, this one is slightly ambiguous. On the language side, 
> > > the extension doesn't add an overload; it only specifies that existing 
> > > overload can be used with a different kind of image. On the API side, it 
> > > does extend the set of features. But at the same time if the extended API 
> > > is not supported, it's not possible to create an image in the first place 
> > > and therefore impossible to call write_imagef. So I question the 
> > > usefulness of such macro on the device side. Does this argument convince 
> > > you it should be removed?
> > > it's not possible to create an image in the first place and therefore 
> > > impossible
> > Not quite that right. Referring to [[ 
> > https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#conversion-rules-for-srgba-and-sbgra-images
> >  | this ]]:
> > 
> > read_imagef built-in functions for OpenCL C 2.0 devices do implicit 
> > conversions to RGB for sRGB images. However, implicit conversion from sRGB 
> > to RGB is done on write_imagef only if corresponding extension is 
> > supported. Otherwise, explicit conversions inside a kernel may be required.
> > 
> > I'm agree that this one is kind of confusing and requires some extra 
> > investigation. But I think now we should remove only those extensions which 
> > are only API related for sure.
> Ok, thanks for providing extra information. I agree this deserves extra 
> clarifications.
> 
> I am not sure whether the frontend will be able to perform such conversions 
> since it doesn't have information regarding the channels. The image types are 
> completely opaque. That means that potentially the BIFs can handle the 
> conversion internally. However, I am unclear whether the macro can be useful 
> i.e. whether there is anything that can be done differently at the source 
> level based on its availability in OpenCL kernel code.
> 
> It should be ok to leave this out for now from the clean up unless someone 
> else can help quickly with some more insights.
Thanks for the clarification; this indeed requires further investigations so 
I've removed this part from my patch for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

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

[PATCH] D91531: [RFC][OpenCL] Provide mechanisms for defining extension macros

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

When reading the documentation [1] for -cl-ext (which I've never used so far), 
I've noticed nothing is said about non-standard configurations (such as 
disabling cl_khr_depth_images with CL2.0). Quickly testing this shows that 
options can be specified to produce non-standard behaviour, as shown by 
https://godbolt.org/z/1Yz1Md.

Is it intentional that -cl-ext allows such non-standard behaviour? (This 
question is not necessarily address to @Anastasia.)
/If so/, then these statements

> Defining __undef_cl_khr_depth_images can alter the default behavior of the 
> predefined macro. This is equivalent to passing -cl-ext=-cl_khr_depth_images.

and

> cl_khr_depth_images is a core functionality of CL2.0 and thefore defining 
> __undef_cl_khr_depth_images doesn't modify the default behavior.

are slightly contradicting each other: the approach with __undef macros seems 
to ensure a more conformant behaviour.

I'm mainly asking for clarification in order to know in which direction we want 
to go, as one could also argue the present documentation doesn't imply 
non-standard behaviour is desired and that the current implementation of 
-cl-ext is buggy.

[1] https://clang.llvm.org/docs/UsersManual.html#cmdoption-cl-ext


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

https://reviews.llvm.org/D91531

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

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini updated this revision to Diff 298660.
mantognini added a comment.

Keep cl_khr_byte_addressable_store and cles_khr_int64 out of this PR. Add more 
details on the extensions being removed in the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

Files:
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl
  clang/test/Misc/nvptx.languageOptsOpenCL.cl
  clang/test/Misc/r600.languageOptsOpenCL.cl
  clang/test/SemaOpenCL/extension-version.cl

Index: clang/test/SemaOpenCL/extension-version.cl
===
--- clang/test/SemaOpenCL/extension-version.cl
+++ clang/test/SemaOpenCL/extension-version.cl
@@ -34,16 +34,6 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifndef cl_khr_gl_sharing
-#error "Missing cl_khr_gl_sharing define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-
-#ifndef cl_khr_icd
-#error "Missing cl_khr_icd define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_icd: enable
-
 // Core features in CL 1.1
 
 #ifndef cl_khr_byte_addressable_store
@@ -86,15 +76,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_local_int32_extended_atomics' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 110)
-// Deprecated abvoe 1.0
-#ifndef cl_khr_select_fprounding_mode
-#error "Missing cl_khr_select_fp_rounding_mode define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_select_fprounding_mode: enable
-#endif
-
-
 // Core feature in CL 1.2
 #ifndef cl_khr_fp64
 #error "Missing cl_khr_fp64 define"
@@ -113,24 +94,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_3d_image_writes' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_gl_event
-#error "Missing cl_khr_gl_event define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_event' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_event : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_d3d10_sharing
-#error "Missing cl_khr_d3d10_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d10_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d10_sharing : enable
-
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
 #ifndef cles_khr_int64
 #error "Missing cles_khr_int64 define"
@@ -140,60 +103,6 @@
 #endif
 #pragma OPENCL EXTENSION cles_khr_int64 : enable
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_context_abort
-#error "Missing cl_context_abort define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_context_abort' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_context_abort : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_d3d11_sharing
-#error "Missing cl_khr_d3d11_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d11_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d11_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_dx9_media_sharing
-#error "Missing cl_khr_dx9_media_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_dx9_media_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_dx9_media_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_image2d_from_buffer
-#error "Missing cl_khr_image2d_from_buffer define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_image2d_from_buffer' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_image2d_from_buffer : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_initialize_memory
-#error "Missing cl_khr_initialize_memory define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_initialize_memory' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_initialize_memory : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_gl_depth_images
-#error "Missing cl_khr_gl_depth_images define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_depth_images' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_depth_images : enable
-
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
 #ifndef cl_khr_gl_msaa_sharing
 #error "Missing cl_khr_gl_msaa_sharing define"
@@ -203,33 +112,6 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_gl_msaa_sharing : e

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.
Herald added a subscriber: dexonsmith.

I don't want to stop the wider discussion, that being said I think I've 
addressed the comment regarding the content of this PR. Let me know if the 
latest version is fine or needs further addressing. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

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

[PATCH] D89372: [OpenCL] Remove unused extensions

[Marco Antognini via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa779a169931c: [OpenCL] Remove unused extensions (authored by 
mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372

Files:
  clang/include/clang/Basic/OpenCLExtensions.def
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl
  clang/test/Misc/nvptx.languageOptsOpenCL.cl
  clang/test/Misc/r600.languageOptsOpenCL.cl
  clang/test/SemaOpenCL/extension-version.cl

Index: clang/test/SemaOpenCL/extension-version.cl
===
--- clang/test/SemaOpenCL/extension-version.cl
+++ clang/test/SemaOpenCL/extension-version.cl
@@ -34,16 +34,6 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifndef cl_khr_gl_sharing
-#error "Missing cl_khr_gl_sharing define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-
-#ifndef cl_khr_icd
-#error "Missing cl_khr_icd define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_icd: enable
-
 // Core features in CL 1.1
 
 #ifndef cl_khr_byte_addressable_store
@@ -86,15 +76,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_local_int32_extended_atomics' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_C_VERSION__) && __OPENCL_C_VERSION__ < 110)
-// Deprecated abvoe 1.0
-#ifndef cl_khr_select_fprounding_mode
-#error "Missing cl_khr_select_fp_rounding_mode define"
-#endif
-#pragma OPENCL EXTENSION cl_khr_select_fprounding_mode: enable
-#endif
-
-
 // Core feature in CL 1.2
 #ifndef cl_khr_fp64
 #error "Missing cl_khr_fp64 define"
@@ -113,24 +94,6 @@
 // expected-warning@-2{{OpenCL extension 'cl_khr_3d_image_writes' is core feature or supported optional core feature - ignoring}}
 #endif
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_gl_event
-#error "Missing cl_khr_gl_event define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_event' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_event : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
-#ifndef cl_khr_d3d10_sharing
-#error "Missing cl_khr_d3d10_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d10_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d10_sharing : enable
-
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 110)
 #ifndef cles_khr_int64
 #error "Missing cles_khr_int64 define"
@@ -140,60 +103,6 @@
 #endif
 #pragma OPENCL EXTENSION cles_khr_int64 : enable
 
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_context_abort
-#error "Missing cl_context_abort define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_context_abort' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_context_abort : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_d3d11_sharing
-#error "Missing cl_khr_d3d11_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_d3d11_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_d3d11_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_dx9_media_sharing
-#error "Missing cl_khr_dx9_media_sharing define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_dx9_media_sharing' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_dx9_media_sharing : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_image2d_from_buffer
-#error "Missing cl_khr_image2d_from_buffer define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_image2d_from_buffer' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_image2d_from_buffer : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_initialize_memory
-#error "Missing cl_khr_initialize_memory define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_initialize_memory' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_initialize_memory : enable
-
-#if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
-#ifndef cl_khr_gl_depth_images
-#error "Missing cl_khr_gl_depth_images define"
-#endif
-#else
-// expected-warning@+2{{unsupported OpenCL extension 'cl_khr_gl_depth_images' - ignoring}}
-#endif
-#pragma OPENCL EXTENSION cl_khr_gl_depth_images : enable
-
 #if (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 120)
 #ifndef cl_khr_gl_msaa_sharing
 #error "Missing cl_khr_gl_msaa_sharing defi

[PATCH] D90385: Address ABI issues introduced with CXCursor_CXXAddrspaceCastExpr

[Marco Antognini via Phabricator via cfe-commits]

mantognini created this revision.
mantognini added reviewers: arphaman, akyrtzi, rsmith, Anastasia.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
mantognini updated this revision to Diff 301579.
mantognini added a comment.
mantognini published this revision for review.

Bump CINDEX_VERSION_MINOR.


mantognini added a comment.

A follow up on https://reviews.llvm.org/D60193.

I don't understand the buildbot failure, 
 I assume it's 
unrelated to this change.

Feel free to add anyone you feel is relevant to review this change.


Revert values in CXCursorKind as they were before
CXCursor_CXXAddrspaceCastExpr was introduced in a6a237f2046a 
 ([OpenCL]
Added addrspace_cast operator in C++ mode., 2020-05-18).

Insert CXCursor_CXXAddrspaceCastExpr after the last expression in
CXCursorKind using the next available value.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90385

Files:
  clang/include/clang-c/Index.h

Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -33,7 +33,7 @@
  * compatible, thus CINDEX_VERSION_MAJOR is expected to remain stable.
  */
 #define CINDEX_VERSION_MAJOR 0
-#define CINDEX_VERSION_MINOR 60
+#define CINDEX_VERSION_MINOR 61
 
 #define CINDEX_VERSION_ENCODE(major, minor) (((major)*1) + ((minor)*1))
 
@@ -2052,62 +2052,58 @@
*/
   CXCursor_CXXFunctionalCastExpr = 128,
 
-  /** OpenCL's addrspace_cast<> expression.
-   */
-  CXCursor_CXXAddrspaceCastExpr = 129,
-
   /** A C++ typeid expression (C++ [expr.typeid]).
*/
-  CXCursor_CXXTypeidExpr = 130,
+  CXCursor_CXXTypeidExpr = 129,
 
   /** [C++ 2.13.5] C++ Boolean Literal.
*/
-  CXCursor_CXXBoolLiteralExpr = 131,
+  CXCursor_CXXBoolLiteralExpr = 130,
 
   /** [C++0x 2.14.7] C++ Pointer Literal.
*/
-  CXCursor_CXXNullPtrLiteralExpr = 132,
+  CXCursor_CXXNullPtrLiteralExpr = 131,
 
   /** Represents the "this" expression in C++
*/
-  CXCursor_CXXThisExpr = 133,
+  CXCursor_CXXThisExpr = 132,
 
   /** [C++ 15] C++ Throw Expression.
*
* This handles 'throw' and 'throw' assignment-expression. When
* assignment-expression isn't present, Op will be null.
*/
-  CXCursor_CXXThrowExpr = 134,
+  CXCursor_CXXThrowExpr = 133,
 
   /** A new expression for memory allocation and constructor calls, e.g:
* "new CXXNewExpr(foo)".
*/
-  CXCursor_CXXNewExpr = 135,
+  CXCursor_CXXNewExpr = 134,
 
   /** A delete expression for memory deallocation and destructor calls,
* e.g. "delete[] pArray".
*/
-  CXCursor_CXXDeleteExpr = 136,
+  CXCursor_CXXDeleteExpr = 135,
 
   /** A unary expression. (noexcept, sizeof, or other traits)
*/
-  CXCursor_UnaryExpr = 137,
+  CXCursor_UnaryExpr = 136,
 
   /** An Objective-C string literal i.e. @"foo".
*/
-  CXCursor_ObjCStringLiteral = 138,
+  CXCursor_ObjCStringLiteral = 137,
 
   /** An Objective-C \@encode expression.
*/
-  CXCursor_ObjCEncodeExpr = 139,
+  CXCursor_ObjCEncodeExpr = 138,
 
   /** An Objective-C \@selector expression.
*/
-  CXCursor_ObjCSelectorExpr = 140,
+  CXCursor_ObjCSelectorExpr = 139,
 
   /** An Objective-C \@protocol expression.
*/
-  CXCursor_ObjCProtocolExpr = 141,
+  CXCursor_ObjCProtocolExpr = 140,
 
   /** An Objective-C "bridged" cast expression, which casts between
* Objective-C pointers and C pointers, transferring ownership in the process.
@@ -2116,7 +2112,7 @@
*   NSString *str = (__bridge_transfer NSString *)CFCreateString();
* \endcode
*/
-  CXCursor_ObjCBridgedCastExpr = 142,
+  CXCursor_ObjCBridgedCastExpr = 141,
 
   /** Represents a C++0x pack expansion that produces a sequence of
* expressions.
@@ -2131,7 +2127,7 @@
* }
* \endcode
*/
-  CXCursor_PackExpansionExpr = 143,
+  CXCursor_PackExpansionExpr = 142,
 
   /** Represents an expression that computes the length of a parameter
* pack.
@@ -2143,7 +2139,7 @@
* };
* \endcode
*/
-  CXCursor_SizeOfPackExpr = 144,
+  CXCursor_SizeOfPackExpr = 143,
 
   /* Represents a C++ lambda expression that produces a local function
* object.
@@ -2157,39 +2153,43 @@
* }
* \endcode
*/
-  CXCursor_LambdaExpr = 145,
+  CXCursor_LambdaExpr = 144,
 
   /** Objective-c Boolean Literal.
*/
-  CXCursor_ObjCBoolLiteralExpr = 146,
+  CXCursor_ObjCBoolLiteralExpr = 145,
 
   /** Represents the "self" expression in an Objective-C method.
*/
-  CXCursor_ObjCSelfExpr = 147,
+  CXCursor_ObjCSelfExpr = 146,
 
   /** OpenMP 5.0 [2.1.5, Array Section].
*/
-  CXCursor_OMPArraySectionExpr = 148,
+  CXCursor_OMPArraySectionExpr = 147,
 
   /** Represents an @available(...) check.
*/
-  CXCursor_ObjCAvailabilityCheckExpr = 149,
+  CXCursor_ObjCAvailabilityCheckExpr = 148,
 
   /**
* Fixed point literal
*/
-  CXCursor_Fi

[PATCH] D60193: [OpenCL] Added addrspace_cast operator

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

I've put up https://reviews.llvm.org/D90385 as a fix for this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60193

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

[PATCH] D90385: Address ABI issues introduced with CXCursor_CXXAddrspaceCastExpr

[Marco Antognini via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbbdbd020d2c2: Address ABI issues introduced with 
CXCursor_CXXAddrspaceCastExpr (authored by mantognini).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90385

Files:
  clang/include/clang-c/Index.h

Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -33,7 +33,7 @@
  * compatible, thus CINDEX_VERSION_MAJOR is expected to remain stable.
  */
 #define CINDEX_VERSION_MAJOR 0
-#define CINDEX_VERSION_MINOR 60
+#define CINDEX_VERSION_MINOR 61
 
 #define CINDEX_VERSION_ENCODE(major, minor) (((major)*1) + ((minor)*1))
 
@@ -2052,62 +2052,58 @@
*/
   CXCursor_CXXFunctionalCastExpr = 128,
 
-  /** OpenCL's addrspace_cast<> expression.
-   */
-  CXCursor_CXXAddrspaceCastExpr = 129,
-
   /** A C++ typeid expression (C++ [expr.typeid]).
*/
-  CXCursor_CXXTypeidExpr = 130,
+  CXCursor_CXXTypeidExpr = 129,
 
   /** [C++ 2.13.5] C++ Boolean Literal.
*/
-  CXCursor_CXXBoolLiteralExpr = 131,
+  CXCursor_CXXBoolLiteralExpr = 130,
 
   /** [C++0x 2.14.7] C++ Pointer Literal.
*/
-  CXCursor_CXXNullPtrLiteralExpr = 132,
+  CXCursor_CXXNullPtrLiteralExpr = 131,
 
   /** Represents the "this" expression in C++
*/
-  CXCursor_CXXThisExpr = 133,
+  CXCursor_CXXThisExpr = 132,
 
   /** [C++ 15] C++ Throw Expression.
*
* This handles 'throw' and 'throw' assignment-expression. When
* assignment-expression isn't present, Op will be null.
*/
-  CXCursor_CXXThrowExpr = 134,
+  CXCursor_CXXThrowExpr = 133,
 
   /** A new expression for memory allocation and constructor calls, e.g:
* "new CXXNewExpr(foo)".
*/
-  CXCursor_CXXNewExpr = 135,
+  CXCursor_CXXNewExpr = 134,
 
   /** A delete expression for memory deallocation and destructor calls,
* e.g. "delete[] pArray".
*/
-  CXCursor_CXXDeleteExpr = 136,
+  CXCursor_CXXDeleteExpr = 135,
 
   /** A unary expression. (noexcept, sizeof, or other traits)
*/
-  CXCursor_UnaryExpr = 137,
+  CXCursor_UnaryExpr = 136,
 
   /** An Objective-C string literal i.e. @"foo".
*/
-  CXCursor_ObjCStringLiteral = 138,
+  CXCursor_ObjCStringLiteral = 137,
 
   /** An Objective-C \@encode expression.
*/
-  CXCursor_ObjCEncodeExpr = 139,
+  CXCursor_ObjCEncodeExpr = 138,
 
   /** An Objective-C \@selector expression.
*/
-  CXCursor_ObjCSelectorExpr = 140,
+  CXCursor_ObjCSelectorExpr = 139,
 
   /** An Objective-C \@protocol expression.
*/
-  CXCursor_ObjCProtocolExpr = 141,
+  CXCursor_ObjCProtocolExpr = 140,
 
   /** An Objective-C "bridged" cast expression, which casts between
* Objective-C pointers and C pointers, transferring ownership in the process.
@@ -2116,7 +2112,7 @@
*   NSString *str = (__bridge_transfer NSString *)CFCreateString();
* \endcode
*/
-  CXCursor_ObjCBridgedCastExpr = 142,
+  CXCursor_ObjCBridgedCastExpr = 141,
 
   /** Represents a C++0x pack expansion that produces a sequence of
* expressions.
@@ -2131,7 +2127,7 @@
* }
* \endcode
*/
-  CXCursor_PackExpansionExpr = 143,
+  CXCursor_PackExpansionExpr = 142,
 
   /** Represents an expression that computes the length of a parameter
* pack.
@@ -2143,7 +2139,7 @@
* };
* \endcode
*/
-  CXCursor_SizeOfPackExpr = 144,
+  CXCursor_SizeOfPackExpr = 143,
 
   /* Represents a C++ lambda expression that produces a local function
* object.
@@ -2157,39 +2153,43 @@
* }
* \endcode
*/
-  CXCursor_LambdaExpr = 145,
+  CXCursor_LambdaExpr = 144,
 
   /** Objective-c Boolean Literal.
*/
-  CXCursor_ObjCBoolLiteralExpr = 146,
+  CXCursor_ObjCBoolLiteralExpr = 145,
 
   /** Represents the "self" expression in an Objective-C method.
*/
-  CXCursor_ObjCSelfExpr = 147,
+  CXCursor_ObjCSelfExpr = 146,
 
   /** OpenMP 5.0 [2.1.5, Array Section].
*/
-  CXCursor_OMPArraySectionExpr = 148,
+  CXCursor_OMPArraySectionExpr = 147,
 
   /** Represents an @available(...) check.
*/
-  CXCursor_ObjCAvailabilityCheckExpr = 149,
+  CXCursor_ObjCAvailabilityCheckExpr = 148,
 
   /**
* Fixed point literal
*/
-  CXCursor_FixedPointLiteral = 150,
+  CXCursor_FixedPointLiteral = 149,
 
   /** OpenMP 5.0 [2.1.4, Array Shaping].
*/
-  CXCursor_OMPArrayShapingExpr = 151,
+  CXCursor_OMPArrayShapingExpr = 150,
 
   /**
* OpenMP 5.0 [2.1.6 Iterators]
*/
-  CXCursor_OMPIteratorExpr = 152,
+  CXCursor_OMPIteratorExpr = 151,
+
+  /** OpenCL's addrspace_cast<> expression.
+   */
+  CXCursor_CXXAddrspaceCastExpr = 152,
 
-  CXCursor_LastExpr = CXCursor_OMPIteratorExpr,
+  CXCursor_LastExpr = CXCursor_CXXAddrspaceCastExpr,
 
   /* Statements */
   CXCursor_FirstStmt = 200,
___
cfe-commits mailing list
cfe-commits@lists.llvm.

[PATCH] D100976: [OpenCL] Simplify use of C11 atomic types

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for the update.


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

https://reviews.llvm.org/D100976

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

[PATCH] D95442: [OpenCL] Add diagnostics for references to functions

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Looks sensible to me.




Comment at: clang/test/SemaOpenCLCXX/members.cl:17
-
-template  struct remove_reference { typedef T type; };
-template  struct remove_reference { typedef T type; };

I wonder, do we lose coverage by removing these templates?

In other words, is the same code for error detection used for templates and 
non-template?



Comment at: clang/test/SemaOpenCLCXX/references.cl:25
+#endif // FPTREXT
+typedef void (&ref2fct_t)();
+#ifndef FPTREXT

This re-uses the same name as above. Could that be an issue?


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

https://reviews.llvm.org/D95442

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

[PATCH] D95442: [OpenCL] Add diagnostics for references to functions

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

Right, thanks for the explanations. They make sense.


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

https://reviews.llvm.org/D95442

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

[PATCH] D96151: [OpenCL] Fix pipe type printing in arg info metadata

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.
This revision is now accepted and ready to land.

Nice refactoring & fix! LGTM. I suppose Stuart's comment about moving 
`typeNameRef` can be addressed when pushing.


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

https://reviews.llvm.org/D96151

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

[PATCH] D96161: [OpenCL] Fix printing of types with signed prefix in arg info metadata

[Marco Antognini via Phabricator via cfe-commits]

mantognini accepted this revision.
mantognini added a comment.

LGTM, thanks. All the comments seem to be addressed.


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

https://reviews.llvm.org/D96161

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