[SYCL][NFC] Detach unit-tests naming from UR #14815
Conversation
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
AlexeySachkov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
This PR will conflict with #14816. I have no strict preference about which one goes first, but I think that if we merge this one first, then I will have less conflicts to resolve. |
Is that s/left/less/ ? |
Yes |
| //==------------- MockDeviceImage.hpp --- UR mock image unit testing library | ||
| //-------==// |
There was a problem hiding this comment.
This needs reformatting.
| @@ -110,17 +112,17 @@ class UrOffloadEntry { | |||
| }; | |||
|
|
|||
| /// Generic array of UR entries. | |||
| template <typename T> class UrArray { | |||
| template <typename T> class Array { | |||
There was a problem hiding this comment.
I'd prefer a different name here... MockEntries, maybe? I'm not sure what exactly this class is doing so can't suggest a better one.
There was a problem hiding this comment.
The thing is that this wrapper not only used for entries, but also for properties and maybe for something else as well. So far I had no other ideas, but I will think about the name more.
There was a problem hiding this comment.
What about ArrayOf? It is still generic, but it is not that close to array, so probably it better conveys that it is a special wrapper that we need
There was a problem hiding this comment.
I'm more concerned by the fact that this wrapper has multiple fields, which is not clear from the name still...
There was a problem hiding this comment.
What about just dropping UrArray completely? I have a POC for it in #15014. Another alternative would be to hide UrArray from unit-tests making it internal to mock objects. I.e. a unit-test won't use UrArray and will pass a regular vector to UrPropertySet, but under the hood that vector will be converted into UrArray
There was a problem hiding this comment.
Another approach here (which I personally like the most) is to separately rename UrArray and "hide" it as an implementation detail: #15604.
@aelovikov-intel, @steffenlarsen, could you please take a look? I know that Steffen has no objections against #15014, but I wonder which approach do you folks think is better here?
There was a problem hiding this comment.
I like #15604 a lot.
This is a first patch in series of detaching unit-test classes from UR names. In a previous attempts to do so (intel#14815) there were concerns about what to do with `UrArray` and this PR is another attempt (see also intel#15014) to do so. This PR renames `UrArray` into `LifetimeExtender` to better communicate its purpose: data structures emitted by the compiler (and therefore used by the runtime) to describe device image and their properties to not store data, but only hold pointers to them. Therefore when we mock those in our unit-tests we need to ensure that their lifetime is long enough to cover the whole test. This is a non-functional change by its spirit, but what used to be `UrArray` is now hidden from writers of unit-tests and the interface is switched to `std::vector` - that is done to hide an implementaiton detail and simplify amount of knowledge required to write unit-tests.
This is a first patch in series of detaching unit-test classes from UR names. In a previous attempts to do so (#14815) there were concerns about what to do with `UrArray` and this PR is another attempt (see also #15014) to do that. This PR renames `UrArray` into `LifetimeExtender` to better communicate its purpose: data structures emitted by the compiler (and therefore used by the runtime) to describe device image and their properties do not store data, but only hold pointers to them. Therefore when we mock those in our unit-tests we need to ensure that their lifetime is long enough to cover the whole test. This is a non-functional change by its spirit, but what used to be `UrArray` is now hidden from writers of unit-tests and the interface is switched to `std::vector` - that is done to hide an implementation detail and simplify amount of knowledge required to write unit-tests.
This is a follow-up from #14145. Our unit tests have mock classes corresponding to device images and other entries our compiler generates. They are all named using
Urprefix, but they have nothing to do with Unified Runtime, they are part of our compiler interface.This patch renames associated files and classes so they do not refer to
Uranymore.This is a mechanical find-and-replace change.