Layer MVP

[MathiasMagnus]

This is an MVP of a layer wrapping the translator library. Some of the features:

• The layer is written such that it should function using 1.x, 2.y and 3.0 runtimes as well.
• It checks whether API queries are supported either being core for the version of the ICD at hand, or whether they are supported due to supporting the cl_khr_il_program extension.
• The layer only alters the behavior of the base ICD when it detects the SPIR-V type ILs are not supported.
  • In such cases, if the layer intercepts compilation but fails to translate, it relays it to the next layer assuming it's a different IL which the ICD likely knows how to handle.

I don't have proper tests in place, but it seems to function fine atop AMD's, Microsoft's and Intel's CPU runtime. Tested on Windows.

(I do have toy CTest support setup on my end, but I'd like to polish it further to make it easier to add new tests and testing more robust in general.)

[kpet]

Thanks for the contribution! That'll make the project much more directly useful to the casual user. I've tried to get it to build and run on my Linux machine and faced a number of issues (see changes on https://github.com/kpet/spirv2clc/tree/layer-fixes):

• We will need to build position independent code for *nix. I've already done that on main since this also made sense for another PR I've just merged. You will want to rebase past those recent changes.
• When running the spirv_new CTS test, I've found a few off-by-one errors in the query handling code (see my branch). After fixing these, the test is still complaining that CL_DEVICE_IL_VERSION_KHR is not supported. What tests have you run? I think passing spirv_new (as much as the translator itself allows that is) is a good target for merging this PR.

Lastly, I've had a cursory look at the code. Nothing really surprising, certainly not in a bad way :). I've left a few comments.

[kpet]

Maybe you could require err to be non-nullptr to avoid having to check everywhere.

[kpet]

Early returns might make the code easier to read.

[MathiasMagnus]

I'm not too happy with the deep nesting of code as well. Personally I'm in favor of making the "happy path" of code as straightforward as possible, and error handling be centrallized, or in some other way "out of sight" not to increase noise. I'm not a big fan of the goto-style error handling what the C samples exhibit, but it does tick all the boxes for me. (It's manual exception handling, really.) I was thinking of using the C++ API here, but that wraps the global namespace API and not the dispatch table. This use case mildly resembles this issue (that refers to the wrapping name, not the wrapped contents), but we may be able to hit two birds with one stone.

[kpet]

I'm not advocating for goto and I don't think you'd need goto's here. I think early returns would work well:

err = call();
if (err != SUCCESS) {
  return err;
}

err = call2();
...

Granted that means the "happy path" is cluttered with error checking code but I think it would still be easier to read.

[MathiasMagnus]

I put some early returns in the code to make it easier to read.

[kpet]

It doesn't practically matter for now but we'll have to think about how to select the OpenCL version used for the translation. If you don't want to address this in this first PR, I suggest you leave a TODO.

[MathiasMagnus]

Added.

[kpet]

I think this is unused

[MathiasMagnus]

Removed. (algorithm is needed, std::copy is invoked.

[kpet]

The else and nesting are unnecessary.

[MathiasMagnus]

Removed.

[kpet]

Just to be clear: that comment applies to several locations in the code. I just pointed out one occurrence.

[MathiasMagnus]

Indeed, I've glossed over the difference in the query name difference when core or KHR. I have little experience running CTS tests, so if you think it's okay, I may add tests which make use of the layer.

[kpet]

I've glossed over the difference in the query name difference when core or KHR.

I think you might need to add support for extension function queries as well. Applications that target the extension will be using these.

I have little experience running CTS tests, so if you think it's okay, I may add tests which make use of the layer.

I'm certainly not opposed to adding more tests :).

[MathiasMagnus]

@kpet Could you elaborate on the off-by-one fixes in this commit? std::string isn't null-terminated, it only is when asking for a c_str() of it (which we don't). The OpenCL spec for CL_DEVICE_EXTENSIONS doesn't mention any null-terminators, in fact the spec mentions null-terminators 6 times only and only in relation to the program sources.

The only error I see in my ways is not accounting for the extra null-terminator I append to the extensions string because I append a literal and that has a null at the end. The best fix I see is param_value_size_ret be string.length() - 1 and std::copy one less char to user storage.

[kpet]

So what I observed and hacked through was (in line order of my commit):

1. After dispatch_extensions.append(" cl_khr_il_program"); the string contained <extensions> \0 cl_khr_program because the NUL character was part of the string data itself due to how the string was initialised. Strings returned by queries are NUL-terminated (checked by the CTS). We talked about clarifying this in the spec, can't remember if we've done it.
2. The size of the storage provided for the implementation to return the list of extensions must have enough space for the NUL character.
3. The size returned to the application must include the NUL character
4. It is valid to call clGetDeviceInfo without a param_value to only get the size.

[kpet]

@MathiasMagnus FYI C++17 is now the default (see #21)

[MathiasMagnus]

I pushed some WIP to show the initiative is not dead. There is an ICD initiate of sorts which is prepared to behave in one of multiple ways to test whether the layer activates when it should. Ultimately it will be able to behave as a 1.x, 2.y and 3.0 ICD with or without core/extension support for SPIR-V. A sample executable will feed SPIR-V programs using the approrpiate APIs and with the layer being put into tracing mode (still missing) it will check whether the layer activated or not as expected.

(It is my first time writing an ICD from scratch, and even the ICD-Loader stub driver isn't the best source of inspiration, as it's doing a lot of extra work, seemingly due to CL/cl_icd.h not having existed when those tests were written.)

[MathiasMagnus]

@kpet Is there anything else to be done here? Last I left this PR here it looked to me everything got addressed. Since then main has progressed to diverge. Were there any comments needing triage? I'll have some time to dedicate to it in the coming days if you can say what's left to do.