[SYCL] Turn on -fsycl-id-queries-fit-in-int by default #3427

smanna12 · 2021-03-26T14:41:55Z

This patch enables sycl_id_queries_fit_in_int macro to true on default mode to fix regressions and performance issue.

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

This patch enables SYCL ID queries fit within MAX_INT option to true on default mode. Signed-off-by: Soumi Manna <[email protected]>

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

clang/include/clang/Driver/Options.td

erichkeane

Seems ok to me.

smanna12 · 2021-03-31T01:02:45Z

Thanks for the reviews.

I am seeing 35 ESIMD tests fail with the patch. The same patch does not cause any failure on downstream. Not sure why? @DenisBakhvalov or @bader, is there any way i could reproduce the failures locally?

http://icl-jenkins.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FLin%2FLLVM_Test_Suite/detail/LLVM_Test_Suite/3023/pipeline

AaronBallman

LGTM aside from precommit failures.

smanna12 · 2021-03-31T20:04:15Z

LGTM aside from precommit failures.

@DenisBakhvalov mentions that, it's because of the llvm.assume intrinsic in device code. BE compiler cannot handle them. So, we cannot enable SYCL_ID_QUERIES_FIT_IN_INT for ESIMD.

DenisBakhvalov · 2021-03-31T20:24:46Z

LGTM aside from precommit failures.

@DenisBakhvalov mentions that, it's because of the llvm.assume intrinsic in device code. BE compiler cannot handle them. So, we cannot enable SYCL_ID_QUERIES_FIT_IN_INT for ESIMD.

Yes, and I'm not sure we can introduce any ESIMD-specific workaround (like we did previously) since we unified the SYCL-ESIMD compilation. I think the way to proceed here is to have vector GPU backend (used by ESIMD code) accept llvm.assume intrinsic. Unfortunately, this means that the current PR is blocked. Tagging @kbobrovs .

kbobrovs · 2021-03-31T22:57:00Z

LGTM aside from precommit failures.

@DenisBakhvalov mentions that, it's because of the llvm.assume intrinsic in device code. BE compiler cannot handle them. So, we cannot enable SYCL_ID_QUERIES_FIT_IN_INT for ESIMD.

Yes, and I'm not sure we can introduce any ESIMD-specific workaround (like we did previously) since we unified the SYCL-ESIMD compilation. I think the way to proceed here is to have vector GPU backend (used by ESIMD code) accept llvm.assume intrinsic. Unfortunately, this means that the current PR is blocked. Tagging @kbobrovs .

ID as int is a big performance improvement for DPC++ in many cases, AFAIK. So, it would be good to have a w/a until ESIMD BE is ready to accepts these. It should be safe w/a to remove llvm.assume in ESIMD kernels for now. I will create a PR shortly.

kbobrovs · 2021-04-01T00:59:17Z

I will create a PR shortly.

#3459

smanna12 · 2021-04-01T02:28:49Z

I will create a PR shortly.

#3459

Thanks @kbobrovs for the w/a.

kbobrovs · 2021-04-01T03:04:05Z

Two PRs should go in before this one, in this order: #3460, #3459

kbobrovs · 2021-04-01T16:39:52Z

@smanna12, please rebase to latest llvm/sycl and rerun, ESIMD failures should be gone

smanna12 · 2021-04-01T16:48:55Z

@smanna12, please rebase to latest llvm/sycl and rerun, ESIMD failures should be gone

Thanks @kbobrovs for the quick fix.

smanna12 · 2021-04-01T22:19:48Z

All ESIMD tests are passing now. This patch is causing regressions for two SubGroup tests.
Failed Tests (2):

SYCL :: SubGroup/sub_group_as.cpp
SYCL :: SubGroup/sub_group_as_vec.cpp

I have created PR for fixing the failing Subgroup tests:
intel/llvm-test-suite#220

http://icl-jenkins.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FLin%2FLLVM_Test_Suite/detail/LLVM_Test_Suite/3132/pipeline

The changes are due to the PR: intel/llvm#3427 Signed-off-by: Soumi Manna <[email protected]>

smanna12 · 2021-04-08T12:45:31Z

Rebasing and retesting PR after intel/llvm-test-suite#220 merge.

smanna12 · 2021-04-08T16:22:54Z

@bader All pre-commit test failures are fixed.

I did not make any change to any of the files on this PR.

I am not sure why buildbot/Lit_with_Cuda is failing with warning like this:
lit WARNING: The PID of hang binary not found.

This worked fine couple of days ago with same patch. Is this expected?

bader · 2021-04-08T16:28:34Z

@bader All pre-commit test failures are fixed.

I did not make any change to any of the files on this PR.

I am not sure why buildbot/Lit_with_Cuda is failing with warning like this:
lit WARNING: The PID of hang binary not found.

This worked fine couple of days ago with same patch. Is this expected?

@tfzhu, is the machine in a broken state?

smanna12 · 2021-04-08T19:40:04Z

@bader All pre-commit test failures are fixed.
I did not make any change to any of the files on this PR.
I am not sure why buildbot/Lit_with_Cuda is failing with warning like this:
lit WARNING: The PID of hang binary not found.
This worked fine couple of days ago with same patch. Is this expected?

@tfzhu, is the machine in a broken state?

Thanks @bader and @tfzhu for fixing the problem.

The changes are due to the PR: intel/llvm#3427 Signed-off-by: Soumi Manna <[email protected]>

…ng (#431) The changes are due to the PR: intel/llvm#3427 Signed-off-by: Soumi Manna <[email protected]>

The changes are due to the PR: intel#3427 Signed-off-by: Soumi Manna <[email protected]>

…ng (intel/llvm-test-suite#431) The changes are due to the PR: intel#3427 Signed-off-by: Soumi Manna <[email protected]>

smanna12 added 3 commits March 26, 2021 07:34

[WIP] enable option to default

f8c9361

This patch enables SYCL ID queries fit within MAX_INT option to true on default mode. Signed-off-by: Soumi Manna <[email protected]>

update patch

508cd1c

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

Merge remote-tracking branch 'my_remote/sycl' into EnableOptionDefault

45cdb9f

smanna12 changed the title ~~[WIP] enable option to default~~ [WIP] Set macro to true on default mode Mar 30, 2021

smanna12 changed the title ~~[WIP] Set macro to true on default mode~~ [SYCL] [WIP] Set macro to true on default mode Mar 30, 2021

Merge remote-tracking branch 'my_remote/sycl' into EnableOptionDefault

4a296a9

smanna12 changed the title ~~[SYCL] [WIP] Set macro to true on default mode~~ [SYCL] Set macro to true on default mode Mar 30, 2021

smanna12 marked this pull request as ready for review March 30, 2021 19:39

smanna12 requested review from AaronBallman, AGindinson, elizabethandrews, mdtoguchi and premanandrao as code owners March 30, 2021 19:39

smanna12 changed the title ~~[SYCL] Set macro to true on default mode~~ [SYCL] Turn on -fsycl_id_queries_fit_in_int by default Mar 30, 2021

smanna12 changed the title ~~[SYCL] Turn on -fsycl_id_queries_fit_in_int by default~~ [SYCL] Turn on -fsycl-id-queries-fit-in-int by default Mar 30, 2021

smanna12 requested a review from erichkeane March 30, 2021 20:49

erichkeane reviewed Mar 30, 2021

View reviewed changes

clang/include/clang/Driver/Options.td Show resolved Hide resolved

erichkeane approved these changes Mar 30, 2021

View reviewed changes

mdtoguchi approved these changes Mar 30, 2021

View reviewed changes

AaronBallman approved these changes Mar 31, 2021

View reviewed changes

kbobrovs mentioned this pull request Apr 1, 2021

[SYCL][ESIMD] Temp w/a for ESIMD BE: filter out @llvm.assume. #3459

Merged

Merge remote-tracking branch 'my_remote/sycl' into EnableOptionDefault

1dcf9fc

smanna12 added a commit to smanna12/llvm-test-suite that referenced this pull request Apr 5, 2021

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

258eec3

The changes are due to the PR: intel/llvm#3427 Signed-off-by: Soumi Manna <[email protected]>

smanna12 added a commit to smanna12/llvm-test-suite that referenced this pull request Apr 5, 2021

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

99281c5

The changes are due to the PR: intel/llvm#3427 Signed-off-by: Soumi Manna <[email protected]>

This was referenced Apr 5, 2021

[SYCL] Update tests with -fno-sycl-id-queries-fit-in-int option intel/llvm-test-suite#220

Merged

[Regression] SubGroup/sub_group_as.cpp, sub_group_as_vec.cpp fails with default ON for sycl-id-queries-fit-in-int #3495

Closed

Merge remote-tracking branch 'my_remote/sycl' into EnableOptionDefault

f6cba3d

bader merged commit f27bb01 into intel:sycl Apr 9, 2021

vladimirlaz pushed a commit to intel/llvm-test-suite that referenced this pull request Jul 13, 2021

[SYCL] Update parallel_for_range.cpp test (#353)

d10f47b

The changes are due to the PR: intel/llvm#3427 Signed-off-by: Soumi Manna <[email protected]>

smaslov-intel pushed a commit to smaslov-intel/llvm-test-suite that referenced this pull request Aug 12, 2021

[SYCL] Update parallel_for_range.cpp test (intel#353)

af2d4a9

The changes are due to the PR: intel/llvm#3427 Signed-off-by: Soumi Manna <[email protected]>

vladimirlaz pushed a commit to intel/llvm-test-suite that referenced this pull request Sep 2, 2021

[SYCL] Update reqd_work_group_size.cpp test with new attribute spelli…

c9e45c8

…ng (#431) The changes are due to the PR: intel/llvm#3427 Signed-off-by: Soumi Manna <[email protected]>

aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023

[SYCL] Update parallel_for_range.cpp test (intel/llvm-test-suite#353)

2d2f1ad

The changes are due to the PR: intel#3427 Signed-off-by: Soumi Manna <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Turn on -fsycl-id-queries-fit-in-int by default #3427

[SYCL] Turn on -fsycl-id-queries-fit-in-int by default #3427

smanna12 commented Mar 26, 2021 •

edited

Loading

erichkeane left a comment

smanna12 commented Mar 31, 2021 •

edited

Loading

AaronBallman left a comment

smanna12 commented Mar 31, 2021

DenisBakhvalov commented Mar 31, 2021

kbobrovs commented Mar 31, 2021

kbobrovs commented Apr 1, 2021

smanna12 commented Apr 1, 2021

kbobrovs commented Apr 1, 2021

kbobrovs commented Apr 1, 2021

smanna12 commented Apr 1, 2021

smanna12 commented Apr 1, 2021 •

edited

Loading

smanna12 commented Apr 8, 2021 •

edited

Loading

smanna12 commented Apr 8, 2021 •

edited

Loading

bader commented Apr 8, 2021

smanna12 commented Apr 8, 2021

[SYCL] Turn on -fsycl-id-queries-fit-in-int by default #3427

[SYCL] Turn on -fsycl-id-queries-fit-in-int by default #3427

Conversation

smanna12 commented Mar 26, 2021 • edited Loading

erichkeane left a comment

Choose a reason for hiding this comment

smanna12 commented Mar 31, 2021 • edited Loading

AaronBallman left a comment

Choose a reason for hiding this comment

smanna12 commented Mar 31, 2021

DenisBakhvalov commented Mar 31, 2021

kbobrovs commented Mar 31, 2021

kbobrovs commented Apr 1, 2021

smanna12 commented Apr 1, 2021

kbobrovs commented Apr 1, 2021

kbobrovs commented Apr 1, 2021

smanna12 commented Apr 1, 2021

smanna12 commented Apr 1, 2021 • edited Loading

smanna12 commented Apr 8, 2021 • edited Loading

smanna12 commented Apr 8, 2021 • edited Loading

bader commented Apr 8, 2021

smanna12 commented Apr 8, 2021

smanna12 commented Mar 26, 2021 •

edited

Loading

smanna12 commented Mar 31, 2021 •

edited

Loading

smanna12 commented Apr 1, 2021 •

edited

Loading

smanna12 commented Apr 8, 2021 •

edited

Loading

smanna12 commented Apr 8, 2021 •

edited

Loading