Skip to content

[SYCL] Add indeterminate constructor to work group memory interface #16003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e574aa7
Add indeterminate constructor to work group memory interface
lbushi25 Nov 6, 2024
8164ff2
Update tests to use indeterminate constructor instead of default one
lbushi25 Nov 6, 2024
543aa6f
Add indeterminate constructor to work group memory interface
lbushi25 Nov 6, 2024
a34b463
Fix typo
lbushi25 Nov 6, 2024
65a8e23
Fix compiler error
lbushi25 Nov 6, 2024
aac82e2
Formatting changes
lbushi25 Nov 6, 2024
d6709ed
Fix unused variable warning
lbushi25 Nov 6, 2024
847163d
Modify handling of SYCL special types to account for default construc…
lbushi25 Nov 8, 2024
f551ba4
Modify handling of SYCL special types to account for default construc…
lbushi25 Nov 8, 2024
172d717
Modify handling of SYCL special types to account for default construc…
lbushi25 Nov 8, 2024
e531b05
Make default constructor private
lbushi25 Nov 8, 2024
52bf8a4
Formatting changes
lbushi25 Nov 8, 2024
32c5cb6
Change naming convention to match rest of the code
lbushi25 Nov 9, 2024
418365d
Merge branch 'intel:sycl' into indeterminate_constructor_work_group_m…
lbushi25 Nov 11, 2024
eb31ca0
Use std::find_if instead of std::for_each to find default constructor
lbushi25 Nov 11, 2024
54abcd0
Revert back changes to default constructor
lbushi25 Nov 12, 2024
21c27fa
Fix formatting errors
lbushi25 Nov 12, 2024
8c94bfc
Revert "Fix formatting errors"
lbushi25 Nov 14, 2024
d5c79ec
Revert "Revert "Fix formatting errors""
lbushi25 Nov 14, 2024
0e3ae8a
Revert "Use std::find_if instead of std::for_each to find default con…
lbushi25 Nov 14, 2024
be386bc
Revert "Revert back changes to default constructor"
lbushi25 Nov 14, 2024
17639ae
Bypass default constructor access specifier in frontend and add tests…
lbushi25 Nov 14, 2024
6cc99d3
Add TODO to revisit the handling of special types
lbushi25 Nov 14, 2024
90bceb0
Formatting changes
lbushi25 Nov 14, 2024
3ea9103
Fix regex in filecheck pattern
lbushi25 Nov 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 40 additions & 14 deletions clang/lib/Sema/SemaSYCL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3950,13 +3950,26 @@ class SyclKernelBodyCreator : public SyclKernelFieldHandler {
}

// Default inits the type, then calls the init-method in the body.
// A type may not have a public default constructor as per its spec so
// typically if this is the case the default constructor will be private and
// in such cases we must manually override the access specifier from private
// to public just for the duration of this default initialization.
// TODO: Revisit this approach once https://github.com/intel/llvm/issues/16061
// is closed.
bool handleSpecialType(FieldDecl *FD, QualType Ty) {
const auto *RecordDecl = Ty->getAsCXXRecordDecl();
AccessSpecifier DefaultConstructorAccess;
auto DefaultConstructor =
std::find_if(RecordDecl->ctor_begin(), RecordDecl->ctor_end(),
[](auto it) { return it->isDefaultConstructor(); });
DefaultConstructorAccess = DefaultConstructor->getAccess();
DefaultConstructor->setAccess(AS_public);

addFieldInit(FD, Ty, std::nullopt,
InitializationKind::CreateDefault(KernelCallerSrcLoc));

DefaultConstructor->setAccess(DefaultConstructorAccess);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually complain during kernel construction if you don't set access?
If yes, does it complain when it should after this change?

Copy link
Contributor Author

@lbushi25 lbushi25 Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually complain during kernel construction if you don't set access? If yes, does it complain when it should after this change?

During the construction of the SYCL Kernel Body, it complains that the constructor has an incorrect number of arguments because it doesn't see a public default constructor. After this change, it doesn't complain anymore during kernel construction but it does complain when using the default constructor in any other place for example if the user uses the default constructor, they will get a compilation error because its not public to them. Its also an approach that works for any special type so we don't have to change our handling based on the special type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually complain during kernel construction if you don't set access? If yes, does it complain when it should after this change?

This line in particular that you have reviewed sets the access back to its original value so that we don't accidentally leave the default constructor visible when it shouldn't be. Frankly, even without this line it seemed like the user could still not call the default constructor if it was defined private, but I chose to err on the safe side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I suppose I would also prefer the safe side.
Shouldn't you be doing the same for each variant of handleSpecialType?

Copy link
Contributor Author

@lbushi25 lbushi25 Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I suppose I would also prefer the safe side. Shouldn't you be doing the same for each variant of handleSpecialType?

Good question. This change is only relevant for the semantic action of the SyclKernelBodyCreator class which has 3 members named handleSpecialType. One is for kernel handler which has its own thing going on so I don't think it needs these changes, the other is the one I've changed here and the remaining one is the one taking a CXXBaseSpecifier. I suppose I should add it to this one too but I forgot, is that what you were asking about?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose I should add it to this one too but I forgot, is that what you were asking about?

Right. Passing precommit also suggests lack of corresponding tests. I suppose a test should contain work group memory as a base to exercise the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I will add such a test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have another solution but this just feels very hacky and I really doubt community clang will accept this. This is out of the scope of the PR but the design of how we handle initialization of special types probably needs to change if we cannot use a default constructor. @tahonermann please weigh in here.

addFieldMemberExpr(FD, Ty);

const auto *RecordDecl = Ty->getAsCXXRecordDecl();
createSpecialMethodCall(RecordDecl, getInitMethodName(), BodyStmts);
CXXMethodDecl *FinalizeMethod =
getMethodByName(RecordDecl, FinalizeMethodName);
Expand All @@ -3970,9 +3983,17 @@ class SyclKernelBodyCreator : public SyclKernelFieldHandler {
}

bool handleSpecialType(const CXXBaseSpecifier &BS, QualType Ty) {
const auto *RecordDecl = Ty->getAsCXXRecordDecl();
const auto *BaseRecordDecl = BS.getType()->getAsCXXRecordDecl();
AccessSpecifier DefaultConstructorAccess;
auto DefaultConstructor =
std::find_if(BaseRecordDecl->ctor_begin(), BaseRecordDecl->ctor_end(),
[](auto it) { return it->isDefaultConstructor(); });
DefaultConstructorAccess = DefaultConstructor->getAccess();
DefaultConstructor->setAccess(AS_public);

addBaseInit(BS, Ty, InitializationKind::CreateDefault(KernelCallerSrcLoc));
createSpecialMethodCall(RecordDecl, getInitMethodName(), BodyStmts);
DefaultConstructor->setAccess(DefaultConstructorAccess);
createSpecialMethodCall(BaseRecordDecl, getInitMethodName(), BodyStmts);
return true;
}

Expand Down Expand Up @@ -4669,16 +4690,21 @@ class SyclKernelIntHeaderCreator : public SyclKernelFieldHandler {
bool handleSyclSpecialType(const CXXRecordDecl *RD,
const CXXBaseSpecifier &BC,
QualType FieldTy) final {
const auto *AccTy =
cast<ClassTemplateSpecializationDecl>(FieldTy->getAsRecordDecl());
assert(AccTy->getTemplateArgs().size() >= 2 &&
"Incorrect template args for Accessor Type");
int Dims = static_cast<int>(
AccTy->getTemplateArgs()[1].getAsIntegral().getExtValue());
int Info = getAccessTarget(FieldTy, AccTy) | (Dims << 11);
Header.addParamDesc(SYCLIntegrationHeader::kind_accessor, Info,
CurOffset +
offsetOf(RD, BC.getType()->getAsCXXRecordDecl()));
if (isSyclAccessorType(FieldTy)) {
const auto *AccTy =
cast<ClassTemplateSpecializationDecl>(FieldTy->getAsRecordDecl());
assert(AccTy->getTemplateArgs().size() >= 2 &&
"Incorrect template args for Accessor Type");
int Dims = static_cast<int>(
AccTy->getTemplateArgs()[1].getAsIntegral().getExtValue());
int Info = getAccessTarget(FieldTy, AccTy) | (Dims << 11);
Header.addParamDesc(SYCLIntegrationHeader::kind_accessor, Info,
CurOffset +
offsetOf(RD, BC.getType()->getAsCXXRecordDecl()));
} else if (SemaSYCL::isSyclType(FieldTy, SYCLTypeAttr::work_group_memory)) {
addParam(FieldTy, SYCLIntegrationHeader::kind_work_group_memory,
offsetOf(RD, BC.getType()->getAsCXXRecordDecl()));
}
return true;
}

Expand Down
17 changes: 17 additions & 0 deletions clang/test/SemaSYCL/Inputs/sycl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,23 @@ class __SYCL_TYPE(multi_ptr) multi_ptr<T, AS, access::decorated::legacy> {
pointer_t m_Pointer;
};

// Dummy implementation of work_group_memory for use in SemaSYCL tests.
template <typename DataT>
class __attribute__((sycl_special_class))
__SYCL_TYPE(work_group_memory) work_group_memory {

// Default constructor for objects later initialized with __init member.
work_group_memory() = default;

public:
work_group_memory(handler &CGH) {}

void __init(__attribute((opencl_local)) DataT *Ptr) { this->Ptr = Ptr; }
void use() const {}
private:
__attribute((opencl_local)) DataT *Ptr;
};

namespace ext {
namespace oneapi {
namespace experimental {
Expand Down
43 changes: 43 additions & 0 deletions clang/test/SemaSYCL/work_group_memory_inheritance.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -ast-dump -sycl-std=2020 %s | FileCheck %s

// Check that AST is correctly generated for kernel arguments that inherit from work group memory.

#include "sycl.hpp"

sycl::queue myQueue;

struct WorkGroupMemoryDerived :
sycl::work_group_memory<int> {
};

int main() {
myQueue.submit([&](sycl::handler &h) {
WorkGroupMemoryDerived DerivedObject{ h };
h.parallel_for<class kernel>([=] {
DerivedObject.use();
});
});
return 0;
}

// CHECK: FunctionDecl {{.*}}kernel{{.*}} 'void (__local int *)'
// CHECK-NEXT: ParmVarDecl {{.*}}used _arg__base '__local int *'
// CHECK-NEXT: CompoundStmt {{.*}}
// CHECK-NEXT: DeclStmt {{.*}}
// CHECK-NEXT: VarDecl {{.*}} used __SYCLKernel {{.*}} cinit
// CHECK-NEXT: InitListExpr {{.*}}
// CHECK-NEXT: InitListExpr {{.*}} 'WorkGroupMemoryDerived'
// CHECK-NEXT: CXXConstructExpr {{.*}} 'sycl::work_group_memory<int>' 'void () noexcept'
// CHECK-NEXT: CXXMemberCallExpr {{.*}} 'void'
// CHECK-NEXT: MemberExpr {{.*}} 'void (__local int *)' lvalue .__init {{.*}}
// CHECK-NEXT: MemberExpr {{.*}} 'WorkGroupMemoryDerived' lvalue .DerivedObject
// CHECK-NEXT: DeclRefExpr {{.*}} lvalue Var {{.*}} '__SYCLKernel'
// CHECK-NEXT: ImplicitCastExpr {{.*}} '__local int *' <LValueToRValue>
// CHECK-NEXT: DeclRefExpr {{.*}} '__local int *' lvalue ParmVar {{.*}} '_arg__base' '__local int *'
// CHECK-NEXT: CompoundStmt {{.*}}
// CHECK-NEXT: CXXOperatorCallExpr {{.*}} 'void' '()'
// CHECK-NEXT: ImplicitCastExpr {{.*}} 'auto (*)() const -> void' <FunctionToPointerDecay>
// CHECK-NEXT: DeclRefExpr {{.*}}'auto () const -> void' lvalue CXXMethod {{.*}} 'operator()' 'auto () const -> void'
// CHECK-NEXT: ImplicitCastExpr {{.*}}
// CHECK-NEXT: DeclRefExpr {{.*}}lvalue Var {{.*}} '__SYCLKernel'

21 changes: 17 additions & 4 deletions sycl/include/sycl/ext/oneapi/experimental/work_group_memory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ class work_group_memory_impl {
} // namespace detail
namespace ext::oneapi::experimental {

struct indeterminate_t {};
inline constexpr indeterminate_t indeterminate;

template <typename DataT, typename PropertyListT = empty_properties_t>
class __SYCL_SPECIAL_CLASS __SYCL_TYPE(work_group_memory) work_group_memory
: sycl::detail::work_group_memory_impl {
Expand All @@ -46,8 +49,20 @@ class __SYCL_SPECIAL_CLASS __SYCL_TYPE(work_group_memory) work_group_memory
using decoratedPtr = typename sycl::detail::DecoratedType<
value_type, access::address_space::local_space>::type *;

public:
// Frontend requires special types to have a default constructor in order to
// have a uniform way of initializing an object of special type to then call
// the __init method on it. This is purely an implementation detail and not
// part of the spec.
// TODO: Revisit this once https://github.com/intel/llvm/issues/16061 is
// closed.
work_group_memory() = default;

#ifdef __SYCL_DEVICE_ONLY__
void __init(decoratedPtr ptr) { this->ptr = ptr; }
#endif

public:
work_group_memory(const indeterminate_t &) {};
work_group_memory(const work_group_memory &rhs) = default;
work_group_memory &operator=(const work_group_memory &rhs) = default;
template <typename T = DataT,
Expand All @@ -73,9 +88,7 @@ class __SYCL_SPECIAL_CLASS __SYCL_TYPE(work_group_memory) work_group_memory
*ptr = value;
return *this;
}
#ifdef __SYCL_DEVICE_ONLY__
void __init(decoratedPtr ptr) { this->ptr = ptr; }
#endif

private:
decoratedPtr ptr;
};
Expand Down
4 changes: 2 additions & 2 deletions sycl/test-e2e/WorkGroupMemory/swap_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ template <typename T> void swap_scalar(T &a, T &b) {
syclexp::work_group_memory<T> temp{cgh};
sycl::nd_range<1> ndr{size, wgsize};
cgh.parallel_for(ndr, [=](sycl::nd_item<1> it) {
syclexp::work_group_memory<T> temp2;
syclexp::work_group_memory<T> temp2{syclexp::indeterminate};
temp2 = temp; // temp and temp2 have the same underlying data
temp = acc_a[0];
acc_a[0] = acc_b[0];
Expand Down Expand Up @@ -264,7 +264,7 @@ void swap_array_2d(T (&a)[N][N], T (&b)[N][N], size_t batch_size) {
const auto j = it.get_global_id()[1];
temp[i][j] = acc_a[i][j];
acc_a[i][j] = acc_b[i][j];
syclexp::work_group_memory<T[N][N]> temp2;
syclexp::work_group_memory<T[N][N]> temp2{syclexp::indeterminate};
temp2 = temp;
acc_b[i][j] = temp2[i][j];
});
Expand Down
Loading