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

[L0] Disabling Driver In Order Lists by default #2416

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

nrspruit
Copy link
Contributor

@nrspruit nrspruit commented Dec 4, 2024

  • Due to L0 Driver issues related to performance using driver in order lists, the feature is being disabled for the time being.
  • Once issues with the L0 Drivers implementing this feature are resolved, then this feature will be re-enabled.

- Due to L0 Driver issues related to performance using driver in order
  lists, the feature is being disabled for the time being.
- Once issues with the L0 Drivers implementing this feature are
  resolved, then this feature will be re-enabled.

Signed-off-by: Neil R. Spruit <[email protected]>
@nrspruit nrspruit requested a review from a team as a code owner December 4, 2024 15:52
@nrspruit nrspruit added v0.10.x Include in the v0.10.x release v0.11.x Include in the v0.11.x release labels Dec 4, 2024
@github-actions github-actions bot added the level-zero L0 adapter specific issues label Dec 4, 2024
nrspruit added a commit to nrspruit/llvm that referenced this pull request Dec 4, 2024
@EwanC
Copy link
Contributor

EwanC commented Dec 4, 2024

In command-buffer code we use ZE_COMMAND_LIST_FLAG_IN_ORDER command-lists without a precondition on this useDriverInOrderLists() check (it's based on a flag passed from SYCL-RT)

ZeCommandListDesc.flags = IsInOrder ? ZE_COMMAND_LIST_FLAG_IN_ORDER
Is that something that will suffer from the problem that's motivating this change?

@Bensuo
Copy link
Contributor

Bensuo commented Dec 4, 2024

In command-buffer code we use ZE_COMMAND_LIST_FLAG_IN_ORDER command-lists without a precondition on this useDriverInOrderLists() check (it's based on a flag passed from SYCL-RT)

ZeCommandListDesc.flags = IsInOrder ? ZE_COMMAND_LIST_FLAG_IN_ORDER

Is that something that will suffer from the problem that's motivating this change?

We do the same check here that's removed in this PR to confirm whether we can use inorder command lists or not, so I would guess that should also be changed.

@nrspruit
Copy link
Contributor Author

nrspruit commented Dec 4, 2024

In command-buffer code we use ZE_COMMAND_LIST_FLAG_IN_ORDER command-lists without a precondition on this useDriverInOrderLists() check (it's based on a flag passed from SYCL-RT)

ZeCommandListDesc.flags = IsInOrder ? ZE_COMMAND_LIST_FLAG_IN_ORDER

. Is that something that will suffer from the problem that's motivating this change?

If this is based on a flag passed from SYCL RT, then it should not be changed. This is about changing the default behavior to use standard lists and not in order by default.

@nrspruit
Copy link
Contributor Author

nrspruit commented Dec 4, 2024

Ah, I missed the "canBeInOrder" let me change that too.

@nrspruit nrspruit requested a review from a team as a code owner December 4, 2024 17:33
nrspruit added a commit to nrspruit/llvm that referenced this pull request Dec 4, 2024
@nrspruit
Copy link
Contributor Author

nrspruit commented Dec 4, 2024

@EwanC , please check the command buffer changes, now both are disabled for now.

@github-actions github-actions bot added the command-buffer Command Buffer feature addition/changes/specification label Dec 4, 2024
@nrspruit nrspruit force-pushed the disable_driver_inorderlists branch 2 times, most recently from bdc4c74 to e42dab4 Compare December 4, 2024 17:37
nrspruit added a commit to nrspruit/llvm that referenced this pull request Dec 4, 2024
@nrspruit
Copy link
Contributor Author

nrspruit commented Dec 4, 2024

@EwanC , please check the command buffer changes, now both are disabled for now.

@EwanC
had a problem in my patch, please re-check, unfortunately this will disable driver in order in command buffer for now until the driver issues can be resolved.

@nrspruit nrspruit force-pushed the disable_driver_inorderlists branch from e42dab4 to 1ee6774 Compare December 4, 2024 17:46
nrspruit added a commit to nrspruit/llvm that referenced this pull request Dec 4, 2024
@nrspruit
Copy link
Contributor Author

nrspruit commented Dec 4, 2024

@EwanC / @Bensuo please re-review so we can get this change in as soon as possible to create the release branch.

This updated PR will disable in both common code and command buffers.

@Bensuo
Copy link
Contributor

Bensuo commented Dec 4, 2024

@EwanC / @Bensuo please re-review so we can get this change in as soon as possible to create the release branch.

This updated PR will disable in both common code and command buffers.

I'm not sure offhand why we don't have this already, but could you add the check for UR_L0_USE_DRIVER_INORDER_LISTS to canBeInOrder() so that we can still enable them if needed? Since that's still in the general implementation I think that should be fine for now.

@nrspruit
Copy link
Contributor Author

nrspruit commented Dec 4, 2024

@EwanC / @Bensuo please re-review so we can get this change in as soon as possible to create the release branch.
This updated PR will disable in both common code and command buffers.

I'm not sure offhand why we don't have this already, but could you add the check for UR_L0_USE_DRIVER_INORDER_LISTS to canBeInOrder() so that we can still enable them if needed? Since that's still in the general implementation I think that should be fine for now.

Ok, let me add that in such that in both cases it is "possible" to use the feature.

nrspruit added a commit to nrspruit/llvm that referenced this pull request Dec 4, 2024
Copy link
Contributor

@Bensuo Bensuo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making those changes!

@nrspruit
Copy link
Contributor Author

nrspruit commented Dec 4, 2024

@EwanC / @Bensuo please re-review so we can get this change in as soon as possible to create the release branch.
This updated PR will disable in both common code and command buffers.

I'm not sure offhand why we don't have this already, but could you add the check for UR_L0_USE_DRIVER_INORDER_LISTS to canBeInOrder() so that we can still enable them if needed? Since that's still in the general implementation I think that should be fine for now.

Ok, let me add that in such that in both cases it is "possible" to use the feature.

@EwanC , I have updated the check in command buffer such that it also checks the env for driver in order. This will enable both paths to work now.

@nrspruit nrspruit force-pushed the disable_driver_inorderlists branch from b7c7f2e to 447b638 Compare December 4, 2024 18:23
nrspruit added a commit to nrspruit/llvm that referenced this pull request Dec 4, 2024
@nrspruit nrspruit added the ready to merge Added to PR's which are ready to merge label Dec 4, 2024
@nrspruit
Copy link
Contributor Author

nrspruit commented Dec 4, 2024

This change is ready to merge, but we are fighting CI issues with a very long delay in execution.

@nrspruit nrspruit merged commit b23a973 into oneapi-src:main Dec 4, 2024
57 of 71 checks passed
nrspruit added a commit that referenced this pull request Dec 4, 2024
[L0] Disabling Driver In Order Lists by default
nrspruit added a commit that referenced this pull request Dec 4, 2024
- Disable driver in order lists by default unless set by env for L0
  adapter.
- #2416

Signed-off-by: Neil R. Spruit <[email protected]>
nrspruit added a commit to nrspruit/llvm that referenced this pull request Dec 4, 2024
@EwanC
Copy link
Contributor

EwanC commented Dec 5, 2024

Sorry @nrspruit I logged off for the day after leaving that comment so didn't see your replies, but Ben has reviewed this on behalf of the command-buffer group and happy with the final patch.

martygrant pushed a commit to intel/llvm that referenced this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command-buffer Command Buffer feature addition/changes/specification level-zero L0 adapter specific issues ready to merge Added to PR's which are ready to merge v0.10.x Include in the v0.10.x release v0.11.x Include in the v0.11.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants