Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] Update tests with -fno-sycl-id-queries-fit-in-int option #220

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

smanna12
Copy link

@smanna12 smanna12 commented Apr 5, 2021

The changes are due to the PR: intel/llvm#3427

Signed-off-by: Soumi Manna [email protected]

@smanna12 smanna12 changed the title Update sub group test [SYCL] Update tests with -fno-sycl-id-queries-fit-in-int option Apr 5, 2021
@smanna12 smanna12 marked this pull request as ready for review April 5, 2021 19:44
@smanna12 smanna12 requested a review from vladimirlaz April 5, 2021 19:44
@smanna12
Copy link
Author

smanna12 commented Apr 5, 2021

The failures seem unrelated to my change.

@@ -1,4 +1,4 @@
// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out
// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple -fno-sycl-id-queries-fit-in-int %s -o %t.out

Choose a reason for hiding this comment

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

I wonder, why those changes are needed at all?

Copy link
Author

@smanna12 smanna12 Apr 6, 2021

Choose a reason for hiding this comment

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

Thanks @AlexeySachkov for reviewing the patch.

intel/llvm#3427: Enables the option -fsycl-id-queries-fit-in-int to true by default. sub_group_as.cpp passes when the option is false on default mode but fails with default ON. So I added -fno-sycl-id-queries-fit-in-int to disable the option and match with the old behavior.

sycl-id-queries-fit-in-int makes a big impact on performance for DPC++ in many cases. The test failures currently block intel/llvm#3427. Is there any workaround i can use here? Thanks.

Choose a reason for hiding this comment

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

It looks like root cause for failing test is the same with post-commit failures after merging intel/llvm#3427

Copy link
Author

Choose a reason for hiding this comment

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

@vladimirlaz, PR: intel/llvm#3427: which has not been merged yet. I am waiting for this PR to be merged first before merging PR: intel/llvm#3427

@smanna12
Copy link
Author

smanna12 commented Apr 6, 2021

@smanna12, please create issue for the regression. It looks like the test start to fail after changing default behavior.
In short: load/store functions for local pointer stop work as expected.

@vladimirlaz, I have created intel/llvm#3495 for the regression and assigned to you. Thanks.

@smanna12
Copy link
Author

smanna12 commented Apr 7, 2021

@smanna12, please create issue for the regression. It looks like the test start to fail after changing default behavior.
In short: load/store functions for local pointer stop work as expected.

@vladimirlaz, I have created intel/llvm#3495 for the regression and assigned to you. Thanks.

@@vladimirlaz, Any update on this? Thanks. Blocks PR: intel/llvm#3427

Copy link

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

I'm okay to submit this patch since we have an issue filed to fix the regression

@smanna12
Copy link
Author

smanna12 commented Apr 7, 2021

Thanks @AlexeySachkov for the review. @vladimirlaz, could you please commit it?. Thanks.

@vladimirlaz vladimirlaz merged commit 5ae35ad into intel:intel Apr 8, 2021
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants