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

Add experimental extension for counter-based events #221

Merged
merged 6 commits into from
Oct 13, 2023

Conversation

jandres742
Copy link

Resolves: #145

@jandres742
Copy link
Author

this is how rendered would look like:

image

@jandres742 jandres742 added enhancement New feature or request API: Core labels Sep 19, 2023
@BartoszDunajski
Copy link

I think you should add more details about usage restrictions

etors:
- name: IMMEDIATE
desc: "Counter-based event is used for immediate command lists (default)"
- name: NON_REGULAR

Choose a reason for hiding this comment

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

do you mean NON_IMMEDIATE here?

Copy link
Author

Choose a reason for hiding this comment

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

thanks @MichalMrozek . Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need two flags? say if we just have IMMEDIATE, then the absence of that flag would mean REGULAR cmdlists?

Copy link
Author

Choose a reason for hiding this comment

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

thanks @aravindksg . I was thinking on that. However, it is better to be explicit. in any case, as you see, we are saying that default value is zero, which is IMMEDAITE, so the user only needs to pass a flag (NON_REGULAR) if they want for regular.

type: $x_event_counter_based_exp_flags_t
      name: flags
      desc: |
            [in] mode flags.
            must be 0 (default)

Signed-off-by: Jaime Arteaga <[email protected]>
@jandres742
Copy link
Author

@MichalMrozek , @BartoszDunajski : please take a look at latest changes.

Signed-off-by: Jaime Arteaga <[email protected]>
scripts/core/EXT_Exp_CounterBasedEvents.rst Outdated Show resolved Hide resolved
scripts/core/EXT_Exp_CounterBasedEvents.rst Outdated Show resolved Hide resolved
scripts/core/EXT_Exp_CounterBasedEvents.rst Outdated Show resolved Hide resolved
scripts/core/EXT_Exp_CounterBasedEvents.rst Outdated Show resolved Hide resolved
desc: "Counter-based event is used for non-immediate command lists"
--- #--------------------------------------------------------------------------
type: struct
desc: "Event descriptor for counter-based events. This structure may be passed to $xEventCreate as pNext member of $x_event_desc_t."
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to consider adding flag for event pools for counter based events? i.e, would there be value in creating an event pool where events from that pool are only used as counting events?

Choose a reason for hiding this comment

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

Yes we need flag for event memory pool as those events doesn't require pool memory to be allocated for them, the storage is inherited from command list.

Copy link
Author

Choose a reason for hiding this comment

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

@MichalMrozek : sorry, could you clarify the comment about event pool? do we want the counter-based flags to be part of the zeEventPoolCreate or zeEventCreate, or both?

Choose a reason for hiding this comment

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

Counter Based Events doesn't need memory backing if they are for immediate command lists, so this must be event pool flag as this is steered to control where allocation happens.
I do not see a need for event flag.
So only change in zeEventPoolCreate flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems to me that we are talking about "counter events" being associated at a command queue level. so would it be better to describe a different struct for this entirely so we can cleanly associate at command queue/command list level rather than shoe-horning it into existing event(pool) struct/desc?

we have fences described here as being associated at command queue level, so something similar for "counter events"?
https://spec.oneapi.io/level-zero/latest/core/PROG.html#fences

Choose a reason for hiding this comment

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

We want to leverage events, otherwise all append API's would need to be rewritten.

Copy link
Author

Choose a reason for hiding this comment

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

thanks @MichalMrozek . I have moved the flags to the event pool logic. Please check.

Signed-off-by: Jaime Arteaga <[email protected]>
@jandres742
Copy link
Author

@MichalMrozek , @aravindksg, @BartoszDunajski : please see latest changes.

scripts/core/EXT_Exp_CounterBasedEventPools.rst Outdated Show resolved Hide resolved
scripts/core/counterbasedeventpool.yml Outdated Show resolved Hide resolved
scripts/core/counterbasedeventpool.yml Show resolved Hide resolved
scripts/core/counterbasedeventpool.yml Show resolved Hide resolved
scripts/core/EXT_Exp_CounterBasedEvents.rst Outdated Show resolved Hide resolved
i.e., a signaled counter-based event always represents the completion of the last call to which it was passed as signal event.
- Synchronizing on a counter-based event waits only the last saved counter value from the last command list that incremented it.
- A counter-based event may be passed as signaling event for a new append call without needing to wait for the signaling of
the last call where it was used.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to add notes on effect of "Timestamp" flags with counter events? I think timestamp flags (even if set) would be ignored right? or we want to forbid user from setting it and return error?

also- do we expect IPC events to continue to work with counter events?

Copy link
Author

Choose a reason for hiding this comment

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

timestamps and IPC are orthogonal features, I dont think they cannot work, However, if that's the case, we can add note in the documentation in future follow ups.

Signed-off-by: Jaime Arteaga <[email protected]>
Signed-off-by: Jaime Arteaga <[email protected]>
@BartoszDunajski
Copy link

In general looks ok. But usage on Regular CmdLists is not well described here. This is something that may be confusing in the future and its worth to explain it.

Counter assigned to Event is updated on Each "execute" call.

For example:

CL1->appendLaunchKernel();
CL1->appendLaunchKernel(signal: Event1);
CL1->appendLaunchKernel();
CL1->close();

CmdQ->execute(CL1); // counter {1 -> 2 -> 3}
CmdQ->execute(CL1); // counter {4 -> 5 -> 6}
CL2-> appendLaunchKernel(wait: Event1); 
CL2->close();
CmdQ->execute(CL2); // wait for counter == 5

CLs executed again:

CmdQ->execute(CL1); // counter {7 -> 8 -> 9}
CmdQ->execute(CL2); // wait for counter == 8

@jandres742
Copy link
Author

In general looks ok. But usage on Regular CmdLists is not well described here. This is something that may be confusing in the future and its worth to explain it.

Counter assigned to Event is updated on Each "execute" call.

For example:

CL1->appendLaunchKernel();
CL1->appendLaunchKernel(signal: Event1);
CL1->appendLaunchKernel();
CL1->close();

CmdQ->execute(CL1); // counter {1 -> 2 -> 3}
CmdQ->execute(CL1); // counter {4 -> 5 -> 6}
CL2-> appendLaunchKernel(wait: Event1); 
CL2->close();
CmdQ->execute(CL2); // wait for counter == 5

CLs executed again:

CmdQ->execute(CL1); // counter {7 -> 8 -> 9}
CmdQ->execute(CL2); // wait for counter == 8

thanks @BartoszDunajski . Let me add that clarification in a follow-up patch.

@wdamon-intel wdamon-intel merged commit 2b30942 into oneapi-src:master Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Core enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

New in-order Event type
5 participants