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 targets to run CTS tests with validation layer. #613

Closed
aarongreig opened this issue Jun 14, 2023 · 9 comments
Closed

Add targets to run CTS tests with validation layer. #613

aarongreig opened this issue Jun 14, 2023 · 9 comments
Assignees
Labels
conformance Conformance test suite issues.
Milestone

Comments

@aarongreig
Copy link
Contributor

It would be useful if we had build targets that ran the CTS tests with validation enabled, rather than having to remember to set the env variables manually.

@aarongreig aarongreig added the conformance Conformance test suite issues. label Jun 14, 2023
@aarongreig aarongreig self-assigned this Jun 14, 2023
@kbenzie
Copy link
Contributor

kbenzie commented Jun 14, 2023

I think we should always enable the validation layer when running conformance, not sure we need to have targets which don't run with validation layer enabled though.

This also came up in intel/llvm#9512 (comment)

@aarongreig
Copy link
Contributor Author

maybe we could add a way to enable validation through the API? that would solve this neatly

@kbenzie
Copy link
Contributor

kbenzie commented Jun 14, 2023

In the style of Vulkan?

@aarongreig
Copy link
Contributor Author

that would be a good model if we're expecting a lot more specialized/third party layers down the line, otherwise we could just do something like extend urInit to take some kind of layer flags/properties

@kbenzie
Copy link
Contributor

kbenzie commented Jun 14, 2023

I like the idea of programatically enabling validation. It's a clean way to avoid the build system while requiring it being enabled in the CTS and also enables parallel langauge runtimes to choose how and if they want to enable validation rather than us being opinionated about it.

@pbalcer
Copy link
Contributor

pbalcer commented Jun 15, 2023

I agree, this makes sense, but like @aarongreig said, we will need to extend urInit to be more flexible. This might also be useful for other advanced configurations. (like #220 or #355).

Something like this:

ur_config_handle_t config;
urConfigNew(&config);


urEnableLayer(config, "validation_core");
urEnableLayer(config, "validation_leaks");


urInit(config);

urTearDown(config);

urConfigDestroy(config);

Or maybe through varargs:

urInit(UR_DEVICE_INIT_FLAG_GPU, UR_PARAM_LAYER("validation_core"), UR_PARAM_LAYER("validation_leaks"), UR_PARAM_END);

?

I prefer the first approach with the "builder" pattern since creating bindings to higher-level languages is easier. Any other ideas? I'll create a new issue Done.

@kbenzie
Copy link
Contributor

kbenzie commented Jun 15, 2023

Slight preference for the builder pattern but why does it require allocating an object with dynamic lifetime?

@pbalcer
Copy link
Contributor

pbalcer commented Jun 15, 2023

Having config be an opaque handle allows us flexibility to extend it in the future without changing existing APIs. We can just add new config-related functions. The easiest way to do that is to make config a heap-allocated object. In this example I chose to use the same nomenclature as we use for other dynamically allocated entities in the API for consistency, but it might imply wrong things. Maybe:

ur_config_handle_t config;
urConfigCreate(&config);

...

urInit(config); // init takes ownership of config, a'la unique_ptr or passing by value in Rust.

@aarongreig
Copy link
Contributor Author

CTS now always enables validation as of #681

@kbenzie kbenzie added this to the 0.7 milestone Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance Conformance test suite issues.
Projects
None yet
Development

No branches or pull requests

3 participants