[SYCL][UR][Bindless][Doc] Fix copy docs and implementation.

[przemektmalon]

This patch fixes the implementation of bindless image copies. Previously, source and destination pitch values were not being set correctly.

This patch also updates the wording around the requirements for ext_oneapi_copy.

A missing requirement was added to the specification. Namely that the CopyExtent parameter in the ext_oneapi_copy functions that take it, must not have 0 values in any of the three dimensions, they must be greater than or equal to 1.

The requirements for ext_oneapi_copy have also been re-written to prescribe what the functions expect, instead of providing a list of cases in which the function may fail. This should hopefully make it clearer and more prescriptive, rather than saying the copy function may fail if some condition is not met, we now say that the functions require that certain conditions be met.

The coverage for sub-region copy testing has also been extended to prevent future regressions.

[przemektmalon]

Ping @intel/llvm-reviewers-runtime @intel/unified-runtime-reviewers-level-zero

[przemektmalon]

Friendly ping @intel/llvm-reviewers-runtime @intel/unified-runtime-reviewers-level-zero

[przemektmalon]

Ping @intel/llvm-reviewers-runtime

[steffenlarsen]

Overall I think it looks good, but could you please add some tests?

[ProGTX]

Overall I think it looks good, but could you please add some tests?

@steffenlarsen Would it be OK if we merged this now to capture the feature freeze? We can work on adding a test separately since that should be NFC.

[steffenlarsen]

Overall I think it looks good, but could you please add some tests?

@steffenlarsen Would it be OK if we merged this now to capture the feature freeze? We can work on adding a test separately since that should be NFC.

I am sorry to be the bad guy, but normal procedure would require functional changes to be tested. If this is a reasonable behavioral change then maybe it is still fine after feature freeze, but as it stands I cannot stand by an approval from a quality point-of-view.

[cperkinsintel]

@przemektmalon - I wanted to help out today by writing some tests for this PR. Getting more familiar with the bindless images has been on my To-Do list for ages and it seemed like a sensible entrée. But AFAICT this PR is mostly just internal changes to fill_copy_args and that routine is WELL covered by tests. There are over two dozen calls in the current test-e2e/bindless_images that end up exercising the code in this PR. So I think we can be confident that it doesn't introduce any new bugs.

If further testing is warranted it is probably to ensure that the updated docs match the changes in the implementation. Can you think of any easy way to test those with the normal bindless image I/O ? Or am I going to have to break modularity to conduct checks under impl or detail::, which I'd rather not do. Let me know.

@steffenlarsen - pinging you since this is in response to your request.

[steffenlarsen]

Thanks for the analysis, @cperkinsintel. My main concern is that there are changes to the documented behavior and the changes are not considered NFC by the author. As such, there should be some observable difference in functional behavior, yet no tests needed to be changed and no tests were added. I am not necessarily concerned about new bugs as much as untested new functionality.

Reading through the documentation changes again, I would focus on the fact that the ext_oneapi_copy changes from "may fail" to now seemingly require failure under the 11 conditions listed. Did we test this requirement before, despite it being optional? We may have, given we control the implementation.

[przemektmalon]

Thanks @cperkinsintel for looking into this. While you are correct that the ext_oneapi_copy and by extension fill_copy_args is quite well covered, we did identify a few gaps that were caught from DPCT translated code. As you mention this PR does not introduce any regressions, the current tests pass, and the DPCT reproducers previously failing are also passing now.

The reason I did not include tests in this PR to fill the gaps in the copy function coverage was to expedite getting these fixes in for the DPCT team. This also included the addition of an extra condition on the ext_oneapi_copy function that applies to sub-region variants of the function.

If further testing is warranted it is probably to ensure that the updated docs match the changes in the implementation.

The new condition added to ext_oneapi_copy was always assumed by the implementation, it was just missing from the specification.

I need to look back and see exactly which tests need to be expanded, but IIRC we need to ensure that every variant of sub-region copies are properly tested, which isn't the case right now.

I.e. all of Host to Device, Device to Host, Device to Device are tested with sub-region copies, where the device memory can be either USM or image_mem, and do this for 1D, 2D, and 3D cases. We currently cover some but not all of this.

Can you think of any easy way to test those with the normal bindless image I/O ? Or am I going to have to break modularity to conduct checks under impl or detail::, which I'd rather not do. Let me know.

It should definitely be possible to extend the current sub-region copy tests using the normal Bindless Images copy APIs, without having to make any calls under impl or detail::.

@steffenlarsen:

Reading through the documentation changes again, I would focus on the fact that the ext_oneapi_copy changes from "may fail" to now seemingly require failure under the 11 conditions listed. Did we test this requirement before, despite it being optional? We may have, given we control the implementation.

The only condition added to the ext_oneapi_copy function is condition 11 regarding the CopyExtent. All the other conditions were just re-written into a way I thought was more clear and prescriptive, but their meaning hasn't changed. Also note that not all conditions apply to every variant of ext_oneapi_copy, and that is stated above the list of conditions, where if a condition mentions a parameter not used by variant X of the function, that condition doesn't apply.

As a bug-fix PR, this can still be cherry-picked after code freeze. I'm happy to look back at the gaps in ext_oneapi_copy coverage and introduce/augment test cases to make sure functionality fixed by this PR is tested.

[steffenlarsen]

Thank you for clarifying, @przemektmalon !

[przemektmalon]

@steffenlarsen I've extended the sub-region copy testing coverage which should cover the few gaps we had in testing and prevent future regressions.

[steffenlarsen]

Very nice! Thanks for adding tests. 🚀

[przemektmalon]

@intel/llvm-gatekeepers This is ready to merge.