- 
                Notifications
    You must be signed in to change notification settings 
- Fork 794
[SYCL][E2E] Fix enum parameter free function kernel test failure #20423
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
base: sycl
Are you sure you want to change the base?
Conversation
| 
 Accessors are passed by-value into  llvm/sycl/include/sycl/handler.hpp Lines 1650 to 1656 in 889db98 
 But they implement common reference semantics, meaning that internally we probably don't pass an actual object, but simply copy a reference to an underlying actual object. I'm fine with merging this PR to re-enable a test for the sake of free function kernels test coverage, but I do not agree that this is a fix, it is a workaround. I do not see anything in the specification saying that this code is invalid. 
 I think that this is a bug in our runtime implementation, not in the test. We should submit a dedicated issue about this | 
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.
Should we drop UNSUPPORTED directive to re-enable the test?
@lbushi25, I suggest that we do that to reinstate our testing coverage an open another tracker for the issue with default-constructed accessors to follow-up on it.
| 
 Sure, let's do that. I will open an issue to to investigate the true cause of the failure. | 
Fix of #20225.
I think that the issue here is that the default accessor is being passed by reference rather than by value to the
set_argfunction which causes the handler object to sporadically have a corrupted copy of the accessor during itsfinalizefunction where some final checks are done. I suspect that with other types of accessors that are constructed using the command group handler, we can declare the accessor at any scope we want because some internal deep copy of the accessor is stored within the handler whereas with default constructed accessors we need a scope that will not die before the handler. This would explain the sporadic nature of the bug.An easy fix is to move the accessor outside the scope of the command group. Since this is a host-side issue, I played around with Valgrind and indeed, before the fix, I was seeing several reports of memory access inside a deallocated region that I traced back to the default constructed accessors and after the fix, nothing, so fingers crossed.
Closes #20225