RFC: CTS integration without using binaries for precompiling shaders

[samuelig]

This is a RFC to gather feedback about CTS integration without requiring glslangValidator binary for precompiling the shaders.

This branch sets up the required bits that allow CTS to get the number of GLSL-written stages present in a given shader_test, get their GLSL code, replace them with a SPIR-V binary form and execute them. CTS will compile GLSL shaders using glslang library on run time, or using a prebuilt version of them if glslang is not present.

Right now, text-format SPIR-V shaders are not supported. I will work on that later.

[bpeel]

Thanks for looking at this.

I think in principal it seems like a nice idea. If we go with this approach it might be nice to redesign the API a little. It’s not ideal that it has to parse the script twice just to get the list of shaders the first time. Perhaps instead we could expose the vr_script as an opaque object and then have methods like the following:

struct vr_script *
vr_executor_load_script(struct vr_executor *executor, struct vr_source *source)

enum vr_result
vr_executor_execute_script(struct vr_executor *executor,
                           const struct vr_script *script);

void
vr_script_get_glsl_shaders(struct vr_script *script,
                           /* think of some way to return the shaders */);

void
vr_script_replace_shader(struct vr_script *script,
                         enum vr_shader_stage shader_stage,
                         size_t source_length,
                         const uint32_t *source)

The comment about returning the shaders is because the shader_test scripts can actually have multiple shaders per stage and they all get linked together with glslang. (Admittedly this probably isn’t very useful for VkRunner because it would only be testing glslang’s linking, but I copied the behaviour from Piglit). Returning them in a single array wouldn’t work unless we change this.

Another option which might be simpler to implement could be to just have a callback to compile shaders when needed. If we have a function to set the callback on the executor then it could divert to that to compile when it is specified, or revert to execing glslang if not. Something like this:

typedef bool
(* vr_callback_compile)(enum vr_source_type source_type,
                        enum vr_shader_stage stage,
                        size_t source_length,
                        const uint8_t *source,
                        size_t *binary_length,
                        uint32_t **binary,
                        void *user_data);

void
vr_executor_set_compile_cb(struct vr_executor *executor,
                           vr_callback_compile compile_cb);

What do you think?

[samuelig]

Thanks for looking at this.

I think in principal it seems like a nice idea. If we go with this approach it might be nice to redesign the API a little. It’s not ideal that it has to parse the script twice just to get the list of shaders the first time. Perhaps instead we could expose the vr_script as an opaque object and then have methods like the following:

Right. That makes totally sense to me. I will hack something and let's see.

struct vr_script *
vr_executor_load_script(struct vr_executor *executor, struct vr_source *source)

enum vr_result
vr_executor_execute_script(struct vr_executor *executor,
                           const struct vr_script *script);

void
vr_script_get_glsl_shaders(struct vr_script *script,
                           /* think of some way to return the shaders */);

void
vr_script_replace_shader(struct vr_script *script,
                         enum vr_shader_stage shader_stage,
                         size_t source_length,
                         const uint32_t *source)

The comment about returning the shaders is because the shader_test scripts can actually have multiple shaders per stage and they all get linked together with glslang. (Admittedly this probably isn’t very useful for VkRunner because it would only be testing glslang’s linking, but I copied the behaviour from Piglit). Returning them in a single array wouldn’t work unless we change this.

Right. However, I would like to update CTS CL with something this week, so the reviewers can give opinions on the CTS side of the integration. I can add a note saying that multiple shaders per stage are not currently supported but it is on the pipeline. Hence, I can have some time to implement it while they review the rest of the code.

Another option which might be simpler to implement could be to just have a callback to compile shaders when needed. If we have a function to set the callback on the executor then it could divert to that to compile when it is specified, or revert to execing glslang if not. Something like this:

typedef bool
(* vr_callback_compile)(enum vr_source_type source_type,
                        enum vr_shader_stage stage,
                        size_t source_length,
                        const uint8_t *source,
                        size_t *binary_length,
                        uint32_t **binary,
                        void *user_data);

void
vr_executor_set_compile_cb(struct vr_executor *executor,
                           vr_callback_compile compile_cb);

What do you think?

Although the idea is good, it doesn't make sense to CTS. CTS will do the compilation with the saved shaders in SourceCollections object give as a parameter in initPrograms(). Afterwards, when we iterate over the test, we can get the binary shader (either compiled on run-time or precompiled before). I don't see where we can plug a callback like this and make it work for CTS.

Again, I would like to keep things simple for the moment and investigate future improvements later.

What do you think?

[bpeel]

What do you think?

Ok, if the first API works better for CTS then that is fine by me.

Right. However, I would like to update CTS CL with something this week, so the reviewers can give opinions on the CTS side of the integration. I can add a note saying that multiple shaders per stage are not currently supported but it is on the pipeline. Hence, I can have some time to implement it while they review the rest of the code.

I understand the time pressure but I’m a bit worried that once CTS starts using an API it will be difficult to change it later. If we can think of a more flexible API now maybe it will save us a lot of hassle later. Perhaps something like this?

struct vr_script_shader {
        enum vr_script_source_type source_type;
        enum vr_shader_stage stage;
        size_t source_length;
        const char *source;
};

void
vr_script_get_shaders(const struct vr_script *script,
                      size_t *n_shaders,
                      struct vr_script_shader **shaders);

void
vr_script_replace_shader_stage_binary(struct vr_script *script,
                                      enum vr_shader_stage shader_stage,
                                      size_t source_length,
                                      const uint32_t *source)

We could document that after calling vr_script_get_shaders the caller must free the *shaders pointer with free. If we make the name of the replace function more descriptive like the above then it will be clear that it replaces the entire stage.

[samuelig]

What do you think?

Ok, if the first API works better for CTS then that is fine by me.

Right. However, I would like to update CTS CL with something this week, so the reviewers can give opinions on the CTS side of the integration. I can add a note saying that multiple shaders per stage are not currently supported but it is on the pipeline. Hence, I can have some time to implement it while they review the rest of the code.

I understand the time pressure but I’m a bit worried that once CTS starts using an API it will be difficult to change it later. If we can think of a more flexible API now maybe it will save us a lot of hassle later. Perhaps something like this?

struct vr_script_shader {
        enum vr_script_source_type source_type;
        enum vr_shader_stage stage;
        size_t source_length;
        const char *source;
};

void
vr_script_get_shaders(const struct vr_script *script,
                      size_t *n_shaders,
                      struct vr_script_shader **shaders);

I have not said it before, but I already have something very similar on place, because I needed it for supporting SPIR-V assembly shaders.

void vr_script_replace_shader_stage_binary(struct vr_script script, enum vr_shader_stage shader_stage, size_t source_length, const uint32_t source)

We could document that after calling `vr_script_get_shaders` the caller must free the `*shaders` pointer with `free`. If we make the name of the replace function more descriptive like the above then it will be clear that it replaces the entire stage.

Right, I can add that.

Just to clarify one thing, my comment about pushing something to the CTS CL is to give something to reviewers to check but mentioning that I will push yet another version to the CL with the multiple shaders supports. With that, they can comment of others things that won't change. In any case, if I can get this sorted out soon, that trick is not going to be needed.

Thanks for your comments.

[samuelig]

Forget this, I did something wrong.

[samuelig]

This is an updated version of the API we agreed on. Currently multiple shaders per stage is not supported, neither by CTS nor by VkRunner. The text-based shaders are directly replaced by their compiled alternatives.

If we want to support multiple-shaders per stage, then we probably need to replace the whole list of shaders in vr-script with the one provided by the user, because the new list will have less number of elements. However, it doesn't require a change in the API, just internal changes to VkRunner.

I'm pretty sure, some minor things can be improved. Tell me your opinion about this, to see if I can update the CL tomorrow before the CTS meeting. For example, I have doubts about where to place things like the enum vr_shader_source_type or vr_script_free(), for now I placed them in vr-executor.h.

[bpeel]

Sorry, I don’t know how to use Github 😆 That last comment was meant to be about the commit which moves vr_shader_stage to vr_executor. I’ll just put all of the comments here.

vr_executor_get_shaders: This function doesn’t have anything to do with the vr_executor and instead it solely operates on the script. I think it would be better to move it to vr-script.c. We can make a public header for vr-script.h and rename the current header to vr-script-private.h. This is similar to what is done with vr-source.h and vr-source-private.h. The same goes for vr_executor_get_num_shaders and vr_executor_replace_shaders_stage_binary.

vr_executor_replace_shaders_stage_binary: I think it would be better if this just freed the entire list of shaders for a particular stage and then added a new list node with the binary script. It could even copy out the code in vr_script_free that frees the list for each stage into a static helper function. It doesn’t make sense to leave any other shaders in the stage if there is a binary shader. If the function is moved to vr-script.c as described above then it can use the add_shader function rather than open-coding it.

vr_executor_load_script: I think it would be better if this took a vr_executor parameter instead of allocating its own temporary executor. On the original CTS patches that I posted, there is a global vr_executor on the Context.

vr_script_free: If we make a public vr-script.h header we can move this there instead of vr-executor.h.

There is a vr_strdup function which you can use instead of vr_alloc+memcpy.

Please try to avoid adding tab characters because it makes the patches hard to read, especially as you seem to have your tab width set to something unusual.

I think the patches need to be tidied up a bit because now there are patches that add stuff and later patches that remove or modify them.

[samuelig]

Sorry, I don’t know how to use Github laughing That last comment was meant to be about the commit which moves vr_shader_stage to vr_executor. I’ll just put all of the comments here.

vr_executor_get_shaders: This function doesn’t have anything to do with the vr_executor and instead it solely operates on the script. I think it would be better to move it to vr-script.c. We can make a public header for vr-script.h and rename the current header to vr-script-private.h. This is similar to what is done with vr-source.h and vr-source-private.h. The same goes for vr_executor_get_num_shaders and vr_executor_replace_shaders_stage_binary.

OK

vr_executor_replace_shaders_stage_binary: I think it would be better if this just freed the entire list of shaders for a particular stage and then added a new list node with the binary script. It could even copy out the code in vr_script_free that frees the list for each stage into a static helper function. It doesn’t make sense to leave any other shaders in the stage if there is a binary shader. If the function is moved to vr-script.c as described above then it can use the add_shader function rather than open-coding it.

OK

vr_executor_load_script: I think it would be better if this took a vr_executor parameter instead of allocating its own temporary executor. On the original CTS patches that I posted, there is a global vr_executor on the Context.

OK, I can change the function. However, we need to clarify what to do on CTS side.

vr_script_free: If we make a public vr-script.h header we can move this there instead of vr-executor.h.

There is a vr_strdup function which you can use instead of vr_alloc+memcpy.

Please try to avoid adding tab characters because it makes the patches hard to read, especially as you seem to have your tab width set to something unusual.

Right, if I have time I will add a editorconfig or similar to the project, because my editor automatically picks tabs instead of spaces.

I think the patches need to be tidied up a bit because now there are patches that add stuff and later patches that remove or modify them.

Right, I'm doing that while fixing the rest.

[samuelig]

I think I have not missed anything. Feel free to give any suggestion.

[bpeel]

Thanks, it’s looking very nice now. I just have a few remaining comments:

Add vr_link_replace() function

I don’t think this function is used anymore so we could drop this patch.

Redefine add_shader() to receive a struct vr_script as argument

LGTM

Rename vr-script.h to vr-script-private.h

LGTM

Add vr-script.h

This adds an include of vr-executor.h to vr-script-private.h and vr-script.c. Perhaps this is a leftover from the tidy up. I don’t think it’s needed and it makes a layering violation.

Add vr_script_get_shaders()

Calling vr_strdup here will break for binary shaders with embedded nulls. (I think it was me who told you to use strdup, so my bad, sorry). I think in the end it will need to use vr_alloc with size+1 and then manually add in the terminating null.

The patch adds vr-shader-stage.h to vr-executor.h which seems irrelevant. It is later removed again in the last patch. Probably best to just never add for the sake of tidiness.

Add vr_executor_execute_script()

LGTM

Added vr_executor_load_script() function

LGTM

Move forward declaration of vr_script_free()

LGTM

Add vr_script_get_num_shaders() function

This should probably just be a one-liner that calls vr_list_length.

Add vr_script_replace_shaders_stage_binary() function

This removes all shaders and then replaces them with binary ones. The _stage_ in the name was meant to imply that it replaces only a stage. I think if we want the API to be this way it would be better to just call it vr_script_replace_shaders_binary.

However, my preference would be to keep it as I suggested before, so that calling it only replaces a single stage with a single binary shader. CTS can then call it once for each stage that it wants to replace. It doesn’t make sense to have multiple binary shaders for a stage anyway. If it only took the source and source_length parameters then it wouldn’t require the caller to confusingly set the source_type on the vr_script_shader_code.

Remove vr-shader-stage.h and move its contents to vr-script.h

LGTM

[samuelig]

Thanks, it’s looking very nice now. I just have a few remaining comments:

Add vr_link_replace() function

I don’t think this function is used anymore so we could drop this patch.

Right. Forgot to remove it.

Redefine add_shader() to receive a struct vr_script as argument

LGTM

Rename vr-script.h to vr-script-private.h

LGTM

Add vr-script.h

This adds an include of vr-executor.h to vr-script-private.h and vr-script.c. Perhaps this is a leftover from the tidy up. I don’t think it’s needed and it makes a layering violation.

Right.

Add vr_script_get_shaders()

Calling vr_strdup here will break for binary shaders with embedded nulls. (I think it was me who told you to use strdup, so my bad, sorry). I think in the end it will need to use vr_alloc with size+1 and then manually add in the terminating null.

OK.

The patch adds vr-shader-stage.h to vr-executor.h which seems irrelevant. It is later removed again in the last patch. Probably best to just never add for the sake of tidiness.

Right, I forgot to delete it.

Add vr_executor_execute_script()

LGTM

Added vr_executor_load_script() function

LGTM

Move forward declaration of vr_script_free()

LGTM

Add vr_script_get_num_shaders() function

This should probably just be a one-liner that calls vr_list_length.

OK

Add vr_script_replace_shaders_stage_binary() function

This removes all shaders and then replaces them with binary ones. The _stage_ in the name was meant to imply that it replaces only a stage. I think if we want the API to be this way it would be better to just call it vr_script_replace_shaders_binary.

However, my preference would be to keep it as I suggested before, so that calling it only replaces a single stage with a single binary shader. CTS can then call it once for each stage that it wants to replace. It doesn’t make sense to have multiple binary shaders for a stage anyway. If it only took the source and source_length parameters then it wouldn’t require the caller to confusingly set the source_type on the vr_script_shader_code.

Agreed. This also simplifies code in CTS side.

Remove vr-shader-stage.h and move its contents to vr-script.h

LGTM

Thanks for the feedback!

[samuelig]

I have doubts about the usage of vr_executor_load_script(). I'm keeping it in this patch series but having made vr_config public and make it a pointer of vr_executor, I think it is not needed anymore. Now the caller can call vr_script_load() directly. If you don't think it is worth keeping it, I will remove it before merging the branch.

[bpeel]

Added vr_executor_load_script() function

Yes, I agree that it would make sense to drop this patch if we are going to make vr_shader_load public.

Add vr_script_replace_shaders_stage_binary() function

I still think it would be better if this function just directly took a pointer to the source code and the length, like this:

void
vr_script_replace_shaders_stage_binary(struct vr_script *script,
                                       enum vr_shader_stage stage,
                                       size_t source_length,
                                       const uint32_t *source)

Otherwise I think it’s very confusing for the caller to know how to fill in the vr_script_shader_code because the source_type only has one possible value and the stage is redundantly duplicated as an argument.

Remove vr-shader-stage.h and move its contents to vr-script.h

Sorry for missing this last time, but I don’t think it makes sense to but this in vr-script.h because then there is a circular depedency between vr-script-private.h and vr-pipeline-key.h. Can we not just make vr-shader-stage.h public instead?

Make struct vr_config public and add auxiliary functions

I think it would be good to make a public and private header for this as well and then make vr_config an opaque object. Otherwise it’s messy that we have to make vr-strtof.h a public header when that is an internal implementation detail.

Define config member field of struct vr_executor to be a pointer

vr_executor_free_config is pretty ugly. Can’t the application just keep track of the vr_config pointer and call vr_config_free directly?

I think if we have public functions to set the callbacks on the vr_config we should just remove the ones from the vr_executor. It’ll get kind of messy and confusing to maintain both.

The rest seems good.

[samuelig]

Added vr_executor_load_script() function

Yes, I agree that it would make sense to drop this patch if we are going to make vr_shader_load public.

OK

Add vr_script_replace_shaders_stage_binary() function

I still think it would be better if this function just directly took a pointer to the source code and the length, like this:

void
vr_script_replace_shaders_stage_binary(struct vr_script *script,
                                       enum vr_shader_stage stage,
                                       size_t source_length,
                                       const uint32_t *source)

Otherwise I think it’s very confusing for the caller to know how to fill in the vr_script_shader_code because the source_type only has one possible value and the stage is redundantly duplicated as an argument.

OK

Remove vr-shader-stage.h and move its contents to vr-script.h

Sorry for missing this last time, but I don’t think it makes sense to but this in vr-script.h because then there is a circular depedency between vr-script-private.h and vr-pipeline-key.h. Can we not just make vr-shader-stage.h public instead?

OK

Make struct vr_config public and add auxiliary functions

I think it would be good to make a public and private header for this as well and then make vr_config an opaque object. Otherwise it’s messy that we have to make vr-strtof.h a public header when that is an internal implementation detail.

OK

Define config member field of struct vr_executor to be a pointer

vr_executor_free_config is pretty ugly. Can’t the application just keep track of the vr_config pointer and call vr_config_free directly?

OK

I think if we have public functions to set the callbacks on the vr_config we should just remove the ones from the vr_executor. It’ll get kind of messy and confusing to maintain both.

OK

The rest seems good.

Thanks!

I have uploaded a new version!

[bpeel]

I just noticed this line in vr_script_get_shaders:

shaders[shaders_number].source_length = shader->length + 1;

That should just be shader->length without the + 1, right?

The definition for vr_config_new has an empty parameter list. In C that means that the function can take any amount of parameters instead of meaning it can’t take any. Please change it to vr_config_new(void).

With those two changes it is:

Reviewed-by: Neil Roberts <nroberts@igalia.com>

Please feel free to merge it. Thanks again for working on it.

[samuelig]

I just noticed this line in vr_script_get_shaders:

shaders[shaders_number].source_length = shader->length + 1;

That should just be shader->length without the + 1, right?

Right. I think I got confused with if we want to keep strlen() convention (shader->length) or sizeof() convention. As we have defined it as length, then it should be strlen() convention, i.e. it doesn't count the NULL character.

The definition for vr_config_new has an empty parameter list. In C that means that the function can take any amount of parameters instead of meaning it can’t take any. Please change it to vr_config_new(void).

OK

With those two changes it is:

Reviewed-by: Neil Roberts <nroberts@igalia.com>

Please feel free to merge it. Thanks again for working on it.

Thanks, I will merge it tomorrow morning.