[SYCL] Disable dead arg elimination for free function kernels #19776
Conversation
This reverts commit 34a11ca.
lbushi25
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
lbushi25
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
I've removed reference to the internal bug report from the PR's description. That's not how upstream development should be done. That said, the rest of the description was great and provides enough context for the change, so no additional actions needed. |
|
Something is wrong with codeownership. I'd expect |
… of https://github.com/lbushi25/llvm into disable_dead_arg_elimination_for_free_function_kernels
lbushi25
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
| @@ -93,18 +93,6 @@ void F<float>(int X) { | |||
| volatile float Y = static_cast<float>(X); | |||
| } | |||
|
|
|||
| template <typename... Args> | |||
There was a problem hiding this comment.
Variadic arguments are not allowed in free function kernels as per the spec here.
This case had been passing until now for some random reason, but disabling dead argument elimination causes it to fail.
It's illegal though, so might as well save a headache and remove it entirely.
There was a problem hiding this comment.
parameter pack is different from variadic arguments
There was a problem hiding this comment.
I was referring to line 99. I put the note on line 96 since its the start of the deleted section.
There was a problem hiding this comment.
I was referring to line 99. I put the note on line 96 since its the start of the deleted section.
Just to err on the safe side, I'm tagging @gmlueck. In the section of the spec that I have linked in my first comment in this thread, is the term "variadic argument" referring to the construct used in line 99 of this test that I've deleted or is it referring to some other construct?
There was a problem hiding this comment.
I agree with @aelovikov-intel. The code on line 96 is not a function with variadic arguments. It is a function template with a parameter pack. When the template is instantiated, there is a fixed number of template arguments (defined by this particular instantiation), and therefore the instantiated function has a fixed number of arguments.
Such a function is expected to be a legal free function kernel. If it does not work, we should investigate why.
There was a problem hiding this comment.
Ok, I see. There is already a tracker for this issue. In the meantime, to get this merged, I'm commenting out only this test case and adding a TODO to uncomment it once the tracker is solved. It's of course less than ideal to reduce test coverage but given that its important to have this change for free function kernels and the fact that the bug is exposed by this PR rather than caused by it, I think it's an acceptable compromise in light of the approaching deadline. Also tagging @AlexeySachkov for awareness.
This reverts commit 8723efe.
Co-authored-by: Alexey Sachkov <[email protected]>
lbushi25
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
lbushi25
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
Re-requested review since this is high priority in the context of the next release. |
| // Variadic template functions do not work with free function kernels. See | ||
| // CMPLRLLVM-69528. | ||
| // TODO: Uncomment the following line once the tracker is resolved. | ||
| // test_func<variadic_templated<double>, double>(); |
There was a problem hiding this comment.
I'll leave review of this to @AlexeySachkov.
There was a problem hiding this comment.
Is it a regression from this patch, or is it something previously known?
There was a problem hiding this comment.
Is it a regression from this patch, or is it something previously known?
I believe it's previously known. The failure described by the tracker has been happening since the test was created, it just wasn't caught by pre-commit since it only manifests with optimizations disabled.
| // Variadic template functions do not work with free function kernels. See | ||
| // CMPLRLLVM-69528. | ||
| // TODO: Uncomment the following line once the tracker is resolved. | ||
| // test_func<variadic_templated<double>, double>(); |
There was a problem hiding this comment.
Is it a regression from this patch, or is it something previously known?
Co-authored-by: Alexey Sachkov <[email protected]>
lbushi25
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
This PR disables dead argument elimination for free function kernels. When using a free function kernel, the user sets the arguments manually using
handler::set_argbut if certain arguments are optimized away in case they are not used, the implementation needs to be aware of this and remove the relevantset_argcalls. Instead, its much more feasible to just disable dead arg elimination in this case.