Skip to content
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

[SYCL] Fix hier_par test after pulldown from llvm #1205

Closed
wants to merge 1 commit into from

Conversation

againull
Copy link
Contributor

Signed-off-by: Artur Gainullin artur.gainullin@intel.com

Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

LGTM.

If it breaks once again, need to think about inventing more stable check for ArgShadow update.

// CHECK: %[[TMP:[A-Za-z_0-9]+]] = bitcast %[[GROUP_CLASS]] addrspace(3)* @[[SHADOW]] to i8 addrspace(3)*
// CHECK: %[[OBJ:[A-Za-z_0-9]+]] = bitcast %[[GROUP_CLASS]]* %[[GROUP_OBJ]] to i8*
// CHECK: call void @llvm.memcpy.p3i8.p0i8.i64(i8 addrspace(3)* align [[ALIGN]] %[[TMP]], {{[^,]+}} %[[OBJ]], {{[^)]+}})
// CHECK: @[[SHADOW:[a-zA-Z0-9]+]] = internal unnamed_addr addrspace(3) global %[[GROUP_CLASS:"[^"]+"]] undef, align [[ALIGN:[0-9]+]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is LLVM transformation always uses addrspace(3) for "local" memory? I think it's supposed to be target depended.
If so, I think we should explicitly specify the target device in the RUN command to make sure that "local" memory is represented as addrspace(3).

Why do we care about align attribute?

Suggested change
// CHECK: @[[SHADOW:[a-zA-Z0-9]+]] = internal unnamed_addr addrspace(3) global %[[GROUP_CLASS:"[^"]+"]] undef, align [[ALIGN:[0-9]+]]
// CHECK: @[[SHADOW:[a-zA-Z0-9]+]] = internal unnamed_addr addrspace(3) global %[[GROUP_CLASS:"[^"]+"]] undef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good concern, thx, I think I can create different check labels for different targets to address this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine if you add a TODO and we address this later.

// CHECK: define {{.*}} spir_func void @{{"[^"]+"}}({{[^,]+}}, %[[GROUP_CLASS]]* byval(%[[GROUP_CLASS]]) align {{[0-9]+}} %[[GROUP_OBJ:[A-Za-z_0-9]+]]) {{.*}}!work_group_scope{{.*}} {
// CHECK-NOT: {{^[ \t]*define}}
// CHECK: %[[OBJ:[A-Za-z_0-9]+]] = bitcast %[[GROUP_CLASS]]* %[[GROUP_OBJ]] to i8*
// CHECK: call void @llvm.memcpy.p3i8.p0i8.i64(i8 addrspace(3)* align [[ALIGN]] getelementptr inbounds (%[[GROUP_CLASS]], %[[GROUP_CLASS]] addrspace(3)* @[[SHADOW]], i32 0, i32 0), {{[^,]+}} %[[OBJ]], {{[^)]+}})
Copy link
Contributor

Choose a reason for hiding this comment

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

What are last two checks for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last two checks are needed to ensure that group object which is passed as byval to the 'parallel for work group' is copied to 'shadow' variable which is a global variable in local address space.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've shared with you my private experiments with enabling standard LLVM transformation passes before translation to SPIR-V format. IIRC, these copies from private to local memory are optimized away. The same might happen in the back-end compilers.
I suggest checking if it's the case. If so, the optimized code is not valid SYCL/OpenCL program.

@bader
Copy link
Contributor

bader commented Mar 13, 2020

Replaced by #1306.

@bader bader closed this Mar 13, 2020
@againull againull deleted the hier_par_test_fix branch December 3, 2022 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants