Move serialization functions to include #875

PatKamin · 2023-09-20T14:22:35Z

Related to issue #828 .

C API will be introduced in another PR.

pbalcer · 2023-09-21T09:12:01Z

scripts/templates/print.hpp.mako

-namespace ${x}_params {
+namespace ${x}_print {
+## API functions declarations #################################################
+// RFC: Should is_handle stay in the common dir?


as long as it's in a namespace, I don't see why not. Maybe we can move it to details (see below).

Moved to details

pbalcer · 2023-09-21T09:18:39Z

scripts/templates/print.hpp.mako

-template <typename T> inline void serializePtr(std::ostream &os, const T *ptr);
-template <typename T> inline void serializeFlag(std::ostream &os, uint32_t flag);
-template <typename T> inline void serializeTagged(std::ostream &os, const void *ptr, T value, size_t size);
+template <typename T> UR_DLLEXPORT inline ${x}_result_t UR_APICALL ${x}PtrPrint(std::ostream &os, const T *ptr);


Since this is a print header, my preference would be to name all these urPrintSomething. Makes it easier to find all print functions.

I would prefer to call these functions this way, too. I just adhered to the function naming convention of UR API (https://github.com/oneapi-src/unified-runtime/blob/main/scripts/core/CONTRIB.rst#naming-convention). Can I ignore this convention in the case of a printing API then?

But we now have something like ur_print::urFunctionParamsPrint, which seems a bit excessive. Why not just ur::printFunctionParams() (i.e. rename namespace to ur, remove ur prefix from the function name and possibly start the function name with print)

ur::print::functionParams() or maybe ur::extras::printFunctionParams()?

I think I'd prefer this whole thing be in its own namespace. Hypothetically, we might want to add some more C++ functionality or bindings, and imho it should be possible to use that functionality without having to import all these operator overloads.

Well, you don't have to include ur_print.hpp header if you don't want to. But I like the ur::extras::printFunctionParams() or perhaps (ur::info::printFunctionParams())

pbalcer · 2023-09-21T09:21:22Z

scripts/templates/print.hpp.mako

-template <typename T> inline void serializePtr(std::ostream &os, const T *ptr) {
+///////////////////////////////////////////////////////////////////////////////
+// @brief Print pointer value
+template <typename T> inline ${x}_result_t ${x}PtrPrint(std::ostream &os, const T *ptr) {


This should not be part of the public API. Maybe we should have a ur_print::details namespace?

Moved to a nested namespace

pbalcer · 2023-09-21T09:23:11Z

scripts/templates/print.hpp.mako

-inline int serializeFunctionParams(std::ostream &os, uint32_t function, const void *params) {
+///////////////////////////////////////////////////////////////////////////////
+// @brief Print function parameters
+UR_DLLEXPORT inline ${x}_result_t UR_APICALL ${x}FunctionParamsPrint(std::ostream &os, uint32_t function, const void *params) {


I'm not sure if this is useful outside of urtrace (but I guess someone might want to implement a similar functionality themselves).

Suggested change

UR_DLLEXPORT inline ${x}_result_t UR_APICALL ${x}FunctionParamsPrint(std::ostream &os, uint32_t function, const void *params) {

UR_DLLEXPORT inline ${x}_result_t UR_APICALL ${x}FunctionParamsPrint(std::ostream &os, enum ur_function_t function, const void *params) {

pbalcer · 2023-10-10T11:19:47Z

scripts/Doxyfile

@@ -848,7 +848,8 @@ RECURSIVE              = YES
 # Note that relative paths are relative to the directory from which doxygen is
 # run.

-EXCLUDE                = README.md
+EXCLUDE                = README.md \
+                         ../include/ur_print.hpp


Just occurred to me that these APIs won't have doxygen docs. We should eventually address this.

Added generation of docs for all operators and a function from ::extras.

igchor · 2023-10-10T15:54:58Z

scripts/templates/print.hpp.mako

-template <typename T> inline void serializePtr(std::ostream &os, const T *ptr);
-template <typename T> inline void serializeFlag(std::ostream &os, uint32_t flag);
-template <typename T> inline void serializeTagged(std::ostream &os, const void *ptr, T value, size_t size);
+template <typename T> UR_DLLEXPORT inline ${x}_result_t UR_APICALL ${x}PrintPtr(std::ostream &os, const T *ptr);


Hm, do we actually need UR_DLLEXPORT? All those functions are defined in the header, right?

PatKamin · 2023-10-20T10:39:24Z

Hm, do we actually need UR_DLLEXPORT? All those functions are defined in the header, right?

Replaced with UR_APIEXPORT for functions visibility on Windows. Also removed these macros from functions of the ::details namespace.

PatKamin · 2023-10-20T10:42:39Z

Just occurred to me that these APIs won't have doxygen docs. We should eventually address this.

Added generation of docs for all operators and a function from ::extras.

PatKamin · 2023-10-20T10:43:09Z

Well, you don't have to include ur_print.hpp header if you don't want to. But I like the ur::extras::printFunctionParams() or perhaps (ur::info::printFunctionParams())

Rearanged namespaces in the header.

kbenzie

Sorry I missed the review request last week. These changes are looking good 👍

pbalcer reviewed Sep 21, 2023

View reviewed changes

PatKamin force-pushed the expose-serialization branch 6 times, most recently from b2a32a3 to cbc9f53 Compare October 3, 2023 12:02

PatKamin requested a review from pbalcer October 4, 2023 10:45

pbalcer approved these changes Oct 4, 2023

View reviewed changes

PatKamin mentioned this pull request Oct 5, 2023

[ur] Create printing C API #930

Merged

pbalcer requested review from igchor and kbenzie October 10, 2023 11:15

pbalcer reviewed Oct 10, 2023

View reviewed changes

igchor reviewed Oct 10, 2023

View reviewed changes

PatKamin force-pushed the expose-serialization branch from cbc9f53 to 41bc180 Compare October 20, 2023 10:41

kbenzie approved these changes Oct 20, 2023

View reviewed changes

kbenzie mentioned this pull request Oct 20, 2023

[UR][L0] Propagate errors from USMAllocationMakeResident intel/llvm#11312

Closed

PatKamin requested review from pbalcer and igchor October 20, 2023 11:30

PatKamin force-pushed the expose-serialization branch from df6d992 to 1f2cd55 Compare October 23, 2023 13:00

PatKamin added 3 commits October 23, 2023 15:01

Rename params stringifier

eff8063

Move print operators into the ur_print namespace

eed9899

Move printing C++ API to include

5f0e6d4

PatKamin force-pushed the expose-serialization branch from 1f2cd55 to 5f0e6d4 Compare October 23, 2023 13:11

pbalcer approved these changes Oct 23, 2023

View reviewed changes

pbalcer approved these changes Oct 24, 2023

View reviewed changes

pbalcer merged commit a2d23dc into oneapi-src:main Oct 24, 2023

PatKamin deleted the expose-serialization branch June 26, 2024 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move serialization functions to include #875

Move serialization functions to include #875

PatKamin commented Sep 20, 2023

pbalcer Sep 21, 2023

PatKamin Sep 27, 2023

pbalcer Sep 21, 2023

PatKamin Sep 21, 2023

PatKamin Sep 27, 2023

igchor Oct 10, 2023

pbalcer Oct 10, 2023

igchor Oct 10, 2023

pbalcer Sep 21, 2023

PatKamin Sep 27, 2023

pbalcer Sep 21, 2023

PatKamin Sep 27, 2023

pbalcer Oct 10, 2023

PatKamin Oct 20, 2023

igchor Oct 10, 2023

PatKamin commented Oct 20, 2023

PatKamin commented Oct 20, 2023

PatKamin commented Oct 20, 2023

kbenzie left a comment

	UR_DLLEXPORT inline ${x}_result_t UR_APICALL ${x}FunctionParamsPrint(std::ostream &os, uint32_t function, const void *params) {
	UR_DLLEXPORT inline ${x}_result_t UR_APICALL ${x}FunctionParamsPrint(std::ostream &os, enum ur_function_t function, const void *params) {

Move serialization functions to include #875

Move serialization functions to include #875

Conversation

PatKamin commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatKamin commented Oct 20, 2023

PatKamin commented Oct 20, 2023

PatKamin commented Oct 20, 2023

kbenzie left a comment

Choose a reason for hiding this comment