[OpenCL][PR48896] Fix address space in binding of initializer lists to references

[Anastasia]

Summary

This patch prevents materializing temporaries in the address space of the references they are bind to. The temporaries should always be in the same address space - private for OpenCL.

This has been fixed earlier for the initialization of references with regular literals in https://reviews.llvm.org/D58634 but the initialization list followed slightly different code path.

@ebevhan I only now noticed your comment on the old review and I agree this doesn't look right. I will prepare another patch, just need to create a reproducer first. Sorry about the delay.

[Anastasia]

• Added a test case.

[rjmccall]

clang/lib/Sema/SemaInit.cpp

4297

Should we be rejecting this path immediately if the address space in T1 can't be converted to from the address space of temporaries?

4318

This should only be VK_XValue if we're binding a r-value reference, I think.

[Anastasia]

Added address space compatibility check and improved testing.

[Anastasia]

clang/lib/Sema/SemaInit.cpp

4297

I agree. I have updated the patch and added more test cases.

4318

Yes, I think so too.

[rjmccall]

Thanks, LGTM with the value-kind fix.

clang/lib/Sema/SemaInit.cpp

4318

Did you mean to fix this to conditionally use VK_LValue?

[Anastasia]

Fixed value kind.

clang/lib/Sema/SemaInit.cpp

4318

Sure, I have added it now. Sorry I got confused somehow.

[rjmccall]

Thanks, LGTM.

I think idiomatically we normally just use (void) instead of spelling it out with static_cast, since there's only one possible meaning for a cast to void. I don't care deeply about it; if you make that change, feel free to make it as part of committing.

[Anastasia]

In D95608#2537316, @rjmccall wrote:

Thanks, LGTM.

I think idiomatically we normally just use (void) instead of spelling it out with static_cast, since there's only one possible meaning for a cast to void. I don't care deeply about it; if you make that change, feel free to make it as part of committing.

Thanks, sure I will stick to the existing practices and update this. Thanks!

clang/lib/Sema/SemaInit.cpp

if (RefRelationship >= Sema::Ref_Related) {
         Sequence.RewrapReferenceInitList(cv1T1, InitList);
       return;
     }
 
     // Update the initializer if we've resolved an overloaded function.
     if (Sequence.step_begin() != Sequence.step_end())
       Sequence.RewrapReferenceInitList(cv1T1, InitList);
   }
-
+  // Perform address space compatibility check.
+  QualType cv1T1IgnoreAS = cv1T1;
+  if (T1Quals.hasAddressSpace()) {
+    Qualifiers T2Quals;
+    (void)S.Context.getUnqualifiedArrayType(InitList->getType(), T2Quals);
+    if (!T1Quals.isAddressSpaceSupersetOf(T2Quals)) {
+      Sequence.SetFailed(
+          InitializationSequence::FK_ReferenceInitDropsQualifiers);
+      return;
+    }
+    // Ignore address space of reference type at this point and perform address
+    // space conversion after the reference binding step.
+    cv1T1IgnoreAS =
+        S.Context.getQualifiedType(T1, T1Quals.withoutAddressSpace());
+  }
   // Not reference-related. Create a temporary and bind to that.
-  InitializedEntity TempEntity = InitializedEntity::InitializeTemporary(cv1T1);
+  InitializedEntity TempEntity =
+      InitializedEntity::InitializeTemporary(cv1T1IgnoreAS);
 
   TryListInitialization(S, TempEntity, Kind, InitList, Sequence,
                         TreatUnavailableAsInvalid);
   if (Sequence) {
     if (DestType->isRValueReferenceType() ||
-        (T1Quals.hasConst() && !T1Quals.hasVolatile()))
-      Sequence.AddReferenceBindingStep(cv1T1, /*BindingTemporary=*/true);
-    else
+        (T1Quals.hasConst() && !T1Quals.hasVolatile())) {
+      Sequence.AddReferenceBindingStep(cv1T1IgnoreAS,
+                                       /*BindingTemporary=*/true);
+      if (T1Quals.hasAddressSpace())
+        Sequence.AddQualificationConversionStep(
+            cv1T1, DestType->isRValueReferenceType() ? VK_XValue : VK_LValue);
+    } else
       Sequence.SetFailed(
           InitializationSequence::FK_NonConstLValueReferenceBindingToTemporary);
   }
 }
 
 /// Attempt list initialization (C++0x [dcl.init.list])
 static void TryListInitialization(Sema &S,
                                   const InitializedEntity &Entity,

clang/test/CodeGenOpenCLCXX/addrspace-references.cl

-//RUN: %clang_cc1 %s -cl-std=clc++ -triple spir -emit-llvm -o - | FileCheck %s
+//RUN: %clang_cc1 %s -cl-std=clc++ -triple spir -emit-llvm -o - -O0 | FileCheck %s
+
+typedef short short2 __attribute__((ext_vector_type(2)));
 
 int bar(const unsigned int &i);
-// CHECK-LABEL: define{{.*}} spir_func void @_Z3foov() 
-void foo() {
+
+class C {
+public:
+  void bar(const short2 &);
+};
+
+// CHECK-LABEL: define{{.*}} spir_func void @_Z6scalarv()
+void scalar() {
   // The generic addr space reference parameter object will be bound
   // to a temporary value allocated in private addr space. We need an
   // addrspacecast before passing the value to the function.
   // CHECK: [[REF:%.*]] = alloca i32
   // CHECK: store i32 1, i32* [[REF]]
   // CHECK: [[REG:%[.a-z0-9]+]] ={{.*}} addrspacecast i32* [[REF]] to i32 addrspace(4)*
   // CHECK: call spir_func i32 @_Z3barRU3AS4Kj(i32 addrspace(4)* align 4 dereferenceable(4) [[REG]])
   bar(1);
 }
+
+// Test list initialization
+// CHECK-LABEL: define{{.*}} spir_func void @_Z4listv()
+void list() {
+  C c1;
+// CHECK: [[REF:%.*]] = alloca <2 x i16>
+// CHECK: store <2 x i16> <i16 1, i16 2>, <2 x i16>* [[REF]]
+// CHECK: [[REG:%[.a-z0-9]+]] = addrspacecast <2 x i16>* [[REF]] to <2 x i16> addrspace(4)*
+// CHECK: call {{.*}}void @_ZNU3AS41C3barERU3AS4KDv2_s(%class.C addrspace(4)* {{.*}}, <2 x i16> addrspace(4)*{{.*}} [[REG]])
+  c1.bar({1, 2});
+}

clang/test/SemaOpenCLCXX/address-space-references.cl

 // RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=clc++ -pedantic -verify -fsyntax-only
 
 __global const int& f(__global float &ref) {
   return ref; // expected-error{{reference of type 'const __global int &' cannot bind to a temporary object because of address space mismatch}}
 }
 
 int bar(const __global unsigned int &i); // expected-note{{passing argument to parameter 'i' here}}
 //FIXME: With the overload below the call should be resolved
 // successfully. However, current overload resolution logic
 // can't detect this case and therefore fails.
 int bar(const unsigned int &i);
 
+typedef short short2 __attribute__((ext_vector_type(2)));
+class C {
+public:
+  void gen(const short2 &);
+  void glob(__global const short2 &); //expected-note{{passing argument to parameter here}}
+  void nested_list(const short2 (&)[2]);
+};
+
 void foo() {
   bar(1); // expected-error{{binding reference of type 'const __global unsigned int' to value of type 'int' changes address space}}
+  C c;
+  c.gen({1, 2});
+  c.glob({1, 2}); //expected-error{{binding reference of type 'const __global short2' (vector of 2 'short' values) to value of type 'void' changes address space}}
+  c.nested_list({{1, 2}, {3, 4}});
 }
 
 // Test addr space conversion with nested pointers
 
 extern void nestptr(int *&); // expected-note {{candidate function not viable: no known conversion from '__global int *__private' to '__generic int *__generic &__private' for 1st argument}}
 extern void nestptr_const(int * const &); // expected-note {{candidate function not viable: cannot pass pointer to address space '__constant' as a pointer to address space '__generic' in 1st argument}}
 int test_nestptr(__global int *glob, __constant int *cons, int* gen) {
   nestptr(glob); // expected-error{{no matching function for call to 'nestptr'}}
   // Addr space conversion first occurs on a temporary.
   nestptr_const(glob);
   // No legal conversion between disjoint addr spaces.
   nestptr_const(cons); // expected-error{{no matching function for call to 'nestptr_const'}}
   return *(*cons ? glob : gen);
 }