[SYCL] optimize enqueueImpKernel by inlining #20681
Conversation
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| bool GlobalHandler::isInstanceAlive() { | ||
| return GlobalHandler::getInstancePtr(); | ||
| } | ||
| GlobalHandler *GlobalHandler::RTGlobalObjHandler = new GlobalHandler(); |
There was a problem hiding this comment.
I don't know if this was meant to be NFC changes, but technically this changes when the first object is created. Previously it would only be created if getInstancePtr was called, now it happens at the time of loading the library. I don't think that's necessarily a problem, but so far it is the only functional change I have noticed in this PR and technically it makes users pay for the instance even if they never use the global handler (implicitly). That said, it is hard to think of such a case.
There was a problem hiding this comment.
This is on purpose to avoid slower access to static variables declared inside a method as we discussed in this comment of original PR.
There was a problem hiding this comment.
I don't feel strongly against doing this, but I wonder if it would be cleaner to do it in a separate PR and mark this "[NFCI]". That way, if there are unforeseen issues with doing the initialization earlier, it should hopefully be easier to isolate the cause when bisecting.
There was a problem hiding this comment.
I've updated summary to not miss this change when doing "git log" later. Can we leave as it? All CI already passed.
Hopefully work of extracting this non-NFC change will not be have to be paid at all. Risk is tiny I think. In case of problems we will pay - I will revert, extract & reapply part.
lslusarczyk
force-pushed
the
enqueueImpKernel_inline
branch
from
08e1487 to
baa17a7
Compare
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
changed the title
lslusarczyk
changed the title
2 participants
[functional change]: GlobalHandler instance move from a static variable declared inside a method to global scope. This changes when the first object is created. Previously it would only be created if getInstancePtr was called, now it happens at the time of loading the library. Users pay for the instance even if they never use the global handle.
Motivation is performance - accessing global variable declared inside a function can be slower than to variable in global scope. Explained e.g. here
[non-functional-change]: various functions exising on hot submit-kernel path get inlined
This PR is part of #20668 which was split into two. See plots in that PR desciption for performance comparisons.