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 function ids to the spec #310

Closed
wants to merge 1 commit into from
Closed

Conversation

pbalcer
Copy link
Contributor

@pbalcer pbalcer commented Mar 1, 2023

XPTI function tracing requires "a stable ABI function id" that
is used to programmatically identify a traced entry point.
This patch adds a requirement for functions to have a unique id,
populates that field for existing functions, and creates an enum
with each function id. It also adds a script that automatically
adds those ids if they are missing.

@pbalcer
Copy link
Contributor Author

pbalcer commented Mar 1, 2023

This is a bit janky, so I'm open to other ideas. In SYCL they have an autogenerated enum. Right now in #300 I'm just using a loop counter inside of the template. Both of those implicitly depend on the order of functions in the spec.
I also considered hashing, but making the ids explicit seemed like a better idea.

XPTI function tracing requires "a stable ABI function id" that
is used to programmatically identify a traced entry point.
This patch adds a requirement for functions to have an unique id,
populates that field for existing functions and creates an enum
with each function id. It also adds a script that automatically
adds those ids if they are missing.
infunc = False
hasid = False

lines = f.readlines()
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing the whole file is unnecessary, try having the following loop this way:

for line in f

And later when checking for a newline at EOF:

f.seek(-1,2) 
if f.read() != '\n'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That won't work because the file is being overwritten. I'd have to buffer the output lines, so it would ultimately come down to the same thing perf- or memory-wise (not that it matters whether this executes in 0.05s or 0.1s... :P).

UR_FUNCTION_ENQUEUE_MEM_IMAGE_WRITE = 28,
UR_FUNCTION_ENQUEUE_MEM_IMAGE_COPY = 29,
UR_FUNCTION_ENQUEUE_MEM_BUFFER_MAP = 31,
UR_FUNCTION_ENQUEUE_MEM_UNMAP = 33,
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing 30 and 32, isn't this enum autogenerated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to rebase this many times now, which is very fiddly... In general, ids don't have to be monotonically increasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out it works fine, entry points with ids 30 and 32 are disabled, see #50.

@pbalcer pbalcer requested a review from kbenzie March 2, 2023 13:10
@kbenzie
Copy link
Contributor

kbenzie commented Mar 2, 2023

This is a bit janky, so I'm open to other ideas. In SYCL they have an autogenerated enum. Right now in #300 I'm just using a loop counter inside of the template. Both of those implicitly depend on the order of functions in the spec.
I also considered hashing, but making the ids explicit seemed like a better idea.

In Khronos specifications they have the concept of a registry, this is used for hardware vendors to reserve ranges of enumeration values for vendor extensions among other things. I wonder if it would make sense to have a registry.yml, that would be updated by the script and committed to the repo, to keep these unique IDs consistent over time?

@pbalcer
Copy link
Contributor Author

pbalcer commented Mar 2, 2023

This is a bit janky, so I'm open to other ideas. In SYCL they have an autogenerated enum. Right now in #300 I'm just using a loop counter inside of the template. Both of those implicitly depend on the order of functions in the spec.
I also considered hashing, but making the ids explicit seemed like a better idea.

In Khronos specifications they have the concept of a registry, this is used for hardware vendors to reserve ranges of enumeration values for vendor extensions among other things. I wonder if it would make sense to have a registry.yml, that would be updated by the script and committed to the repo, to keep these unique IDs consistent over time?

I like that idea. The script would be simpler, and such a registry would keep the autogenerated id separate from the actual spec. On the other hand, accessing the id variable in templates will be a little bit more involved, but nothing a helper function can't solve. I'll sleep on it.
Thanks!

Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

Assuming the IDs only need to be consistent in a particular version of the headers and not across versions this looks good.

@pbalcer
Copy link
Contributor Author

pbalcer commented Mar 9, 2023

It needs to be stable across versions as if it's part of the ABI. XPTI-based tools will target this and shouldn't break when, e.g., a new function is added in the middle of some yml file. That's why this version adds the ids to the spec, and the generate script respects existing ids.

I started reworking this in line with your suggestion for ids to be in a separate file. I'll probably close this PR and open a new one.

@pbalcer
Copy link
Contributor Author

pbalcer commented Mar 10, 2023

Closing in favor of a different approach in #343

@pbalcer pbalcer closed this Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants