add UR loader, null adapter and basic example #93

pbalcer · 2022-12-07T12:12:46Z

The components this patch implements are mostly derived from the ones implemented by level-zero.

This patch otherwise makes only the minimal amount of changes required for loader:

adds new CI that builds the loader
updates cmake scripts to build and install the loader
adds generate and check-generated custom cmake targets
adds a tiny hello-world example to verify loader functionality

Co-authored-by: Brandon Yates brandon.yates@intel.com
Co-authored-by: Lisanna Dettwyler lisanna.dettwyler@intel.com
Co-authored-by: Patryk Kaminski patryk.kaminski@intel.com
Co-authored-by: Krzysztof Swiecicki krzysztof.swiecicki@intel.com

PatKamin · 2022-12-07T12:22:00Z

.github/workflows/cmake.yml

+      run: cmake --build ${{github.workspace}}/build --target check-generated --config ${{env.BUILD_TYPE}}
+
+    - name: Build
+      run: cmake --build ${{github.workspace}}/build --config ${{env.BUILD_TYPE}}


For me this --config param doesn't change anything. What is relevant here for setting up debug or release build is the -DCMAKE_BUILD_TYPE argument which you added in the previous step.

I believe --config is important when using the Visual Studio project generator. This will be defaulting to the Unix Makefiles generation though.

I used a template :) I'll remove that config part. Windows is probably going to need its own separate job anyway.

PatKamin · 2022-12-07T12:26:39Z

.gitignore

@@ -53,6 +53,9 @@
 # Debug files
 scripts/**/*.json

+# Python cache
+scripts/templates/__pycache__/


This dir can be generated in scripts/, too. Maybe generalize this to sth like scripts/**/__pycache__/?

PatKamin · 2022-12-07T12:42:52Z

README.md

+
+**General**:
+
+If you've made modifications to the specification, you can also run a custom `generate` target prior to building.


I think it's better to move this section to be one of the first ones, so this will be something that the user will try using first instead of doing this the hard way (by figuring out which script should be run and with which params, see Source Code Generation section and the linked README).

kbenzie · 2022-12-07T12:47:16Z

Its going to take me a whole to review this and I have a busy day today, I'll get back to it tomorrow.

pbalcer · 2022-12-07T12:53:28Z

No rush - I know it's a huge PR, but I think it's the bare minimum that makes sense. Thankfully most of it is automatically generated...

PatKamin · 2022-12-07T12:49:29Z

examples/hello_world/hello_world.cpp

+            }
+            static const size_t DEVICE_NAME_MAX_LEN = 1024;
+            char device_name[DEVICE_NAME_MAX_LEN] = {0};
+            status = urDeviceGetInfo(d, UR_DEVICE_INFO_NAME, DEVICE_NAME_MAX_LEN - 1, static_cast<void *>(&device_name), nullptr);


This can be extended by the check on the actual device name length returned from this function (with a last parameter).

I'd rather keep the example short. Checking the length here doesn't add much - device_name will be a valid NULL-terminated C string, which cout should output correctly.

In a test, on the other hand, we should verify that if this function returns success, the name is populated with a non-zero length string.

PatKamin · 2022-12-07T12:56:58Z

scripts/json2src.py

+            if args.loader:
+                generate_code.generate_loader(srcpath, config['name'], config['namespace'], config['tags'], args.ver, specs, input['meta'])
+            if args.layers:
+                generate_code.generate_layers(srcpath, config['name'], config['namespace'], config['tags'], args.ver, specs, input['meta'])


There is no generate_layers() in generate_code module. Safe to remove this call?

I don't want to remove it entirely since we will be adding the layer functionality soon. I commented out these lines, so it's impossible to call this a non-existent method.

FranklandJack · 2022-12-08T10:08:12Z

source/common/ur_singleton.h

+ * SPDX-License-Identifier: MIT
+ *
+ */
+#pragma once


Although this is probably supported by almost any compiler, I think it is still non-standard, so it might be better to favour a traditional #ifndef guard.

FranklandJack · 2022-12-08T10:09:25Z

source/common/ur_singleton.h

+
+//////////////////////////////////////////////////////////////////////////
+/// a abstract factory for creation of singleton objects
+template<typename _singleton_t, typename _key_t>


I think prefixing types with _ is UB since they are reserved by the compiler.

FranklandJack · 2022-12-08T10:15:24Z

source/common/ur_util.h

+ */
+
+#if defined(__cplusplus)
+#pragma once


FranklandJack · 2022-12-08T10:20:53Z

source/common/ur_util.h

+    env = getenv(name);
+#endif
+
+    if( ( nullptr == env ) || ( 0 == strcmp( "0", env ) ) )


I don't think ( 0 == strcmp( "0", env ) ) is neccesarry here since we do return ( 0 == strcmp( "1", env ) ); on the line below which will return false if env is 0.

Good catch :) Done

FranklandJack · 2022-12-08T10:21:43Z

source/common/ur_singleton.h

+        auto key = getKey( std::forward<Ts>( _params )... );
+
+        if(key == 0) // No zero keys allowed in map
+            return static_cast<_singleton_t*>(0);


My preference would be to always have {} in any control flow statements.

That's my preference as well. But I refrained from making any stylistic changes (the coding style is all over the place :-)). I suggest we pick a single style and enforce it with clang-format (we were planning on doing that soon after this PR was merged).

I can do it with this PR, but a) it might conflict with the generated files, so it's not trivial addition, and b) it would probably affect the entire repo, not just the loader, so might clutter the PR.

As long as we have an issue to track these changes linked here, I'd be happy for them to be addressed in a future PR.

Done. I'll add an issue about adding style checking to the build system.

Addressing them in a future issue works for me 👍

FranklandJack · 2022-12-08T10:39:26Z

source/loader/ur_lib.h

+ * @file ur_lib.h
+ *
+ */
+#pragma once


I think we should use #ifndef here.

FranklandJack · 2022-12-08T10:39:49Z

source/loader/ur_loader.h

+ * SPDX-License-Identifier: MIT
+ *
+ */
+#pragma once


I think we should use #ifndef here.

FranklandJack · 2022-12-08T10:39:59Z

source/loader/ur_object.h

+ * @file ur_object.h
+ *
+ */
+#pragma once


I think we should use #ifndef here.

FranklandJack · 2022-12-08T10:40:13Z

source/loader/windows/lib_init.cpp

+ *
+ */
+
+#include "../ur_lib.h"


Is there a way to avoid relative file paths here?

FranklandJack · 2022-12-08T10:40:20Z

source/loader/windows/loader_init.cpp

+ *
+ */
+
+#include "../ur_loader.h"


.github/workflows/pull_request.yaml

CMakeLists.txt

kbenzie

Besides the minor things which could be addressed later, I think this looks good.

pbalcer · 2022-12-08T14:34:27Z

In addition to the review commits, I needed to address the conflicting workflow change that verified the python module. So I added a test/python/basic.py test that runs in the new cmake workflow. Once #94 is merged, we can modify that test to instantiate the UR_DDI class, and it should just work.

FranklandJack

Thanks for addressing the comments I've made - LGTM.

CMakeLists.txt

The components this patch implements are mostly derived from the ones implemented by level-zero. This patch otherwise makes only the minimal amount of changes required for loader: - adds new CI that builds the loader - updates cmake scripts to build and install the loader - adds generate and check-generated custom cmake targets - adds a tiny hello-world example to verify loader functionality Co-authored-by: Brandon Yates <brandon.yates@intel.com> Co-authored-by: Lisanna Dettwyler <lisanna.dettwyler@intel.com> Co-authored-by: Patryk Kaminski <patryk.kaminski@intel.com> Co-authored-by: Krzysztof Swiecicki <krzysztof.swiecicki@intel.com>

FranklandJack · 2022-12-13T10:25:45Z

.github/workflows/pull_request.yaml

-            git diff
-            exit 1
-          fi
-          cd ../include


Did this check get lost in the move to doing this in CMake?

pbalcer force-pushed the ur-rebase branch from cda1613 to 8a35abe Compare December 7, 2022 12:29

PatKamin reviewed Dec 7, 2022

View reviewed changes

FranklandJack reviewed Dec 8, 2022

View reviewed changes

kbenzie requested changes Dec 8, 2022

View reviewed changes

.github/workflows/pull_request.yaml Outdated Show resolved Hide resolved

kbenzie reviewed Dec 8, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

kbenzie approved these changes Dec 8, 2022

View reviewed changes

pbalcer force-pushed the ur-rebase branch 2 times, most recently from 0a22152 to 05280a9 Compare December 8, 2022 14:31

pbalcer force-pushed the ur-rebase branch 2 times, most recently from 1159f4b to 76173e7 Compare December 8, 2022 15:54

FranklandJack approved these changes Dec 8, 2022

View reviewed changes

pbalcer force-pushed the ur-rebase branch 2 times, most recently from 40e896d to 2b6e5c3 Compare December 8, 2022 16:03

kbenzie reviewed Dec 8, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

pbalcer force-pushed the ur-rebase branch from 2b6e5c3 to a1e7e7c Compare December 8, 2022 17:00

PatKamin approved these changes Dec 9, 2022

View reviewed changes

pbalcer force-pushed the ur-rebase branch from a1e7e7c to eef1a3f Compare December 9, 2022 11:00

pbalcer merged commit 7ef04f9 into oneapi-src:main Dec 9, 2022

FranklandJack reviewed Dec 13, 2022

View reviewed changes

FranklandJack mentioned this pull request Dec 13, 2022

Add cmake machinery to regenerate generated files as part of build when generator sources change #32

Closed


		General:

		If you've made modifications to the specification, you can also run a custom `generate` target prior to building.

add UR loader, null adapter and basic example #93

add UR loader, null adapter and basic example #93

Uh oh!

Conversation

pbalcer commented Dec 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbenzie commented Dec 7, 2022

Uh oh!

pbalcer commented Dec 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbalcer Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbalcer Dec 8, 2022 •

edited

Loading

pbalcer commented Dec 8, 2022 •

edited

Loading