-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix mixed output of adapter and regular traces #2101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really a fan of this approach, I'd prefer to have indents like ltrace. But I guess it solves the immediate problem...
Also, please try to keep the first line of the commit message under 50 characters, and the rest of the message lines under ~70.
$ man git commit
...
Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character)
line summarizing the change, followed by a blank line and then a more thorough description. The text up to the
first blank line in a commit message is treated as the commit title, and that title is used throughout Git.
@@ -40,16 +40,18 @@ __urdlllocal ur_result_t UR_APICALL urAdapterGet( | |||
uint64_t instance = getContext()->notify_begin(UR_FUNCTION_ADAPTER_GET, | |||
"urAdapterGet", ¶ms); | |||
|
|||
getContext()->logger.info("---> urAdapterGet"); | |||
std::ostringstream args_str; | |||
ur::extras::printFunctionParams(args_str, UR_FUNCTION_ADAPTER_GET, ¶ms); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not all the arguments can be accessed here, because some of them may be output arguments, with uninitialized content, which results in UB on access.
We either have to selectively access the parameters, adding an option to printFunctionParams to decide whether we are pre or post function execution, or, we can simply read those arguments after the execution. Which is what the original implementation did, for this exact reason.
30f8e30
to
2d10f85
Compare
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
2b5d716
to
12e0cee
Compare
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
12e0cee
to
cf835bf
Compare
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
cf835bf
to
3785539
Compare
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
3785539
to
1e4088f
Compare
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
1e4088f
to
3a7b9f1
Compare
makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
3a7b9f1
to
439c484
Compare
) makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
) makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
) makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
) makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
439c484
to
5e8592b
Compare
) makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
5e8592b
to
cf5994a
Compare
Fixed oneapi-src#2002 issue. Regular UR tracing prints now calls in two separate lines like PI does.
@pbalcer , can you take a look again to this PR and merge this if all is OK?
E2E tests on llvm pass on my change and corresponding change in llvm is approved, see: intel/llvm#15439 |
) makes llvm work with this change: oneapi-src/unified-runtime#2101 fixes this bug: oneapi-src/unified-runtime#2002
Issue with ugly interleaved Level0 adapter and Level0 Unified Runtime traces, that is oneapi-src/unified-runtime#2002 issue, has been fixed in oneapi-src/unified-runtime#2101. This PR adds necessary changes to e2e tests to make them pass with modified tracing in L0 Adapter. This PR also reverts #15167 which disabled some traces because of the bug which is now fixed, so traces can be restored. --------- Co-authored-by: Piotr Balcer <[email protected]>
Fix #2002 issue.
When merged this PR will make regular UR calls tracing being printed calls in two separate lines like PI does.