[SYCL][UR] Disable UR_LAYER_PARAMETER_VALIDATION by default #15105

nrspruit · 2024-08-15T23:19:29Z

No description provided.

sycl/source/detail/ur.cpp

aelovikov-intel · 2024-08-22T16:42:42Z

sycl/test-e2e/format.py

Do we really need it? We had internal e2e tests running with UR verification/L0 plugin leaks detection enabled (before PI removal) and the only single issue (AFAIK) caught by UR verification level was the verification code itself.

The plugins used to have some level of parameter checking in the entry points themselves, that's not the case with UR where this only occurs on the validation layer. The failing tests in this PR appear to depend on parameter checking for correctness.

I'm still not convinced. I'm not going to "request changes", so if anybody from the SYCL RT feels strongly that this PR should be merged, feel free to approve.

The failing tests in this PR appear to depend on parameter checking for correctness.

can you elaborate on this?

Hello @aelovikov-intel ,

So, I want to understand your concern. My primary concern is to disable the validation layer by default because this is impacting performance on every call to the UR. Is your concern the fact that we would enable the validation layer during E2E testing?

Why would that be an issue? Don't we want to have more checks during the E2E testing?

Why would that be an issue?

because it affects timing and could possible hide issues that our customers would see without that validation. Also, how many issues that actually catches?

sycl/test-e2e/format.py

kbenzie · 2024-08-23T09:23:55Z

sycl/test-e2e/format.py

The plugins used to have some level of parameter checking in the entry points themselves, that's not the case with UR where this only occurs on the validation layer. The failing tests in this PR appear to depend on parameter checking for correctness.

nrspruit · 2024-08-23T18:21:49Z

@intel/llvm-gatekeepers please merge when available.

nrspruit · 2024-08-26T15:07:08Z

@intel/llvm-gatekeepers , please merge.

sarnex · 2024-08-26T15:09:12Z

@nrspruit We still need a review from @intel/llvm-reviewers-runtime

@aelovikov-intel provided initial feedback, maybe he can check if his feedback was addressed and approve if so?

sarnex · 2024-08-26T18:08:31Z

sycl/source/detail/ur.cpp

probably this comment needs to be updated too

- Only Enable UR_LAYER_PARAMETER_VALIDATION if UR_LOG_VALIDATION is set. Signed-off-by: Neil R. Spruit <neil.r.spruit@intel.com>

Signed-off-by: Neil R. Spruit <neil.r.spruit@intel.com>

nrspruit · 2024-08-27T18:47:21Z

@intel/llvm-gatekeepers please merge.

) Signed-off-by: Neil R. Spruit <neil.r.spruit@intel.com>

nrspruit temporarily deployed to WindowsCILock August 15, 2024 23:19 — with GitHub Actions Inactive

nrspruit temporarily deployed to WindowsCILock August 15, 2024 23:45 — with GitHub Actions Inactive

nrspruit marked this pull request as ready for review August 20, 2024 20:41

nrspruit requested a review from a team as a code owner August 20, 2024 20:41

nrspruit requested a review from kbenzie August 20, 2024 20:41

nrspruit force-pushed the dis_ur_val_layer_default branch from 1103b42 to 0f5e04c Compare August 20, 2024 20:43

nrspruit temporarily deployed to WindowsCILock August 20, 2024 20:43 — with GitHub Actions Inactive

nrspruit temporarily deployed to WindowsCILock August 20, 2024 21:09 — with GitHub Actions Inactive

kbenzie reviewed Aug 21, 2024

View reviewed changes

sycl/source/detail/ur.cpp Outdated Show resolved Hide resolved

nrspruit force-pushed the dis_ur_val_layer_default branch from 0f5e04c to 4a7dad1 Compare August 22, 2024 00:26

nrspruit requested a review from a team as a code owner August 22, 2024 00:26

nrspruit requested a review from aelovikov-intel August 22, 2024 00:26

nrspruit temporarily deployed to WindowsCILock August 22, 2024 00:26 — with GitHub Actions Inactive

nrspruit temporarily deployed to WindowsCILock August 22, 2024 00:50 — with GitHub Actions Inactive

aelovikov-intel reviewed Aug 22, 2024

View reviewed changes

kbenzie reviewed Aug 23, 2024

View reviewed changes

nrspruit force-pushed the dis_ur_val_layer_default branch from 4a7dad1 to 6485823 Compare August 23, 2024 15:43

nrspruit had a problem deploying to WindowsCILock August 23, 2024 15:45 — with GitHub Actions Error

nrspruit force-pushed the dis_ur_val_layer_default branch from 6485823 to 904412b Compare August 23, 2024 15:46

nrspruit had a problem deploying to WindowsCILock August 23, 2024 15:47 — with GitHub Actions Error

nrspruit force-pushed the dis_ur_val_layer_default branch from 904412b to 4169e8f Compare August 23, 2024 16:17

nrspruit temporarily deployed to WindowsCILock August 23, 2024 16:18 — with GitHub Actions Inactive

kbenzie approved these changes Aug 23, 2024

View reviewed changes

nrspruit temporarily deployed to WindowsCILock August 23, 2024 17:01 — with GitHub Actions Inactive

nrspruit temporarily deployed to WindowsCILock August 26, 2024 17:43 — with GitHub Actions Inactive

sarnex reviewed Aug 26, 2024

View reviewed changes

sycl/source/detail/ur.cpp Outdated

Copy link

Contributor

sarnex Aug 26, 2024

Choose a reason for hiding this comment

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

probably this comment needs to be updated too

nrspruit had a problem deploying to WindowsCILock August 26, 2024 18:21 — with GitHub Actions Failure

nrspruit temporarily deployed to WindowsCILock August 26, 2024 20:02 — with GitHub Actions Inactive

nrspruit had a problem deploying to WindowsCILock August 26, 2024 20:35 — with GitHub Actions Error

nrspruit force-pushed the dis_ur_val_layer_default branch from 15b9b01 to 63fda4b Compare August 26, 2024 21:32

nrspruit had a problem deploying to WindowsCILock August 26, 2024 21:34 — with GitHub Actions Failure

nrspruit force-pushed the dis_ur_val_layer_default branch from 63fda4b to 0d6980e Compare August 26, 2024 21:50

nrspruit temporarily deployed to WindowsCILock August 26, 2024 21:51 — with GitHub Actions Inactive

nrspruit had a problem deploying to WindowsCILock August 26, 2024 22:42 — with GitHub Actions Error

nrspruit force-pushed the dis_ur_val_layer_default branch from 0d6980e to 0cb0e5f Compare August 26, 2024 23:09

nrspruit temporarily deployed to WindowsCILock August 26, 2024 23:10 — with GitHub Actions Inactive

nrspruit had a problem deploying to WindowsCILock August 26, 2024 23:37 — with GitHub Actions Failure

nrspruit temporarily deployed to WindowsCILock August 27, 2024 00:28 — with GitHub Actions Inactive

nrspruit marked this pull request as draft August 27, 2024 00:33

nrspruit had a problem deploying to WindowsCILock August 27, 2024 00:43 — with GitHub Actions Failure

nrspruit temporarily deployed to WindowsCILock August 27, 2024 00:43 — with GitHub Actions Inactive

nrspruit temporarily deployed to WindowsCILock August 27, 2024 01:06 — with GitHub Actions Inactive

nrspruit had a problem deploying to WindowsCILock August 27, 2024 01:27 — with GitHub Actions Failure

nrspruit had a problem deploying to WindowsCILock August 27, 2024 14:28 — with GitHub Actions Failure

omarahmed1111 force-pushed the dis_ur_val_layer_default branch from 0cb0e5f to 3ded0b1 Compare August 27, 2024 15:36

omarahmed1111 marked this pull request as ready for review August 27, 2024 15:36

omarahmed1111 had a problem deploying to WindowsCILock August 27, 2024 15:37 — with GitHub Actions Error

nrspruit added 2 commits August 27, 2024 08:39

[SYCL][UR] Disable UR_LAYER_PARAMETER_VALIDATION by default

6d80b5f

- Only Enable UR_LAYER_PARAMETER_VALIDATION if UR_LOG_VALIDATION is set. Signed-off-by: Neil R. Spruit <neil.r.spruit@intel.com>

[SYCL][UR] Fully remove enabling the validation layer thru the SYCL init

685c6a9

Signed-off-by: Neil R. Spruit <neil.r.spruit@intel.com>

nrspruit force-pushed the dis_ur_val_layer_default branch from 3ded0b1 to 685c6a9 Compare August 27, 2024 15:40

nrspruit temporarily deployed to WindowsCILock August 27, 2024 15:42 — with GitHub Actions Inactive

aelovikov-intel approved these changes Aug 27, 2024

View reviewed changes

omarahmed1111 mentioned this pull request Aug 27, 2024

[UR][L0] Enable Immediate Command List by default given Intel DG2 #15031

Closed

nrspruit temporarily deployed to WindowsCILock August 27, 2024 16:53 — with GitHub Actions Inactive

sarnex merged commit 6dd3892 into intel:sycl Aug 27, 2024

AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Nov 26, 2024

[SYCL][UR] Disable UR_LAYER_PARAMETER_VALIDATION by default (intel#15105

ec5dd29

) Signed-off-by: Neil R. Spruit <neil.r.spruit@intel.com>

[SYCL][UR] Disable UR_LAYER_PARAMETER_VALIDATION by default #15105

[SYCL][UR] Disable UR_LAYER_PARAMETER_VALIDATION by default #15105

Uh oh!

Conversation

nrspruit commented Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aelovikov-intel Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbenzie Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrspruit Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kbenzie Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

nrspruit commented Aug 23, 2024

Uh oh!

nrspruit commented Aug 26, 2024

Uh oh!

sarnex commented Aug 26, 2024

Uh oh!

sarnex Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

nrspruit commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nrspruit commented Aug 15, 2024 •

edited

Loading

aelovikov-intel Aug 22, 2024 •

edited

Loading

aelovikov-intel Aug 26, 2024 •

edited

Loading