[SYCL] Fix address space for function pointer kernel arguments #5924
Conversation
Commits 57fd86d (https://reviews.llvm.org/D77119) and 4eaf584 (https://reviews.llvm.org/D111566) set address space of function pointers unconditionally to program address space. A follow-up commit ed5b42b (https://reviews.llvm.org/D119045) modified the code to retain explicitly specified address spaces for function pointer. This introduced a regression in cases where function pointers were captured as kernel arguments, since openCL kernel generation sets global address space explicitly to all pointers. This patch reverts the behavior for function pointers captured as kernel arguments. Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
I don't think so, it changes SemaSYCL part which is not upstreamed yet. |
I cannot put this exact change in llorg because we don't have this code in llorg, but I can get equivalent functionality by modifying this patch intead - https://reviews.llvm.org/D119045 i.e instead of changing SemaSYCL, I can add a condition to the change I made in ASTContext.cpp in D119045 to not retain explicit address space when SYCLIsDevice is set. I wasn't sure what the right solution was. I went with this solution because if we make the change in ASTContext, function types which can have explicit address space will not be handled correctly in SYCL. I'm not sure how many exist - one such type was __ptr32. |
| int (*fptr)(); | ||
| int *ptr; | ||
|
|
||
| // define dso_local spir_kernel void @{{.*}}fake_kernel_2{{.*}}(i32 ()* align 4 %_arg_fptr, i32 addrspace(1)* align 4 %_arg_ptr) |
There was a problem hiding this comment.
Being honest I'm not sure what is the correct address space for function pointers in kernel arguments. It would be great to get feedback from @AlexeySachkov .
There was a problem hiding this comment.
Just an FYI, since April 2020 (or whenever this commit was pulled down to DPCPP project https://reviews.llvm.org/D77119), the address space has been 0, which is why there was a regression downstream after https://reviews.llvm.org/D119045. If the correct behavior is some other address space, I think it should be done as a follow-up patch (if its not a quick fix). This patch is just fixing the regression
There was a problem hiding this comment.
Actually I just noticed there are some precommit fails. So maybe this has unintended consequences. I'll take a look.
There was a problem hiding this comment.
https://github.com/intel/llvm/runs/5747268781?check_suite_focus=true <- this failure is not related and fixed by fa054cc.
There was a problem hiding this comment.
OK thanks for letting me know. There is one more failure on Windows - SYCL :: Printf/mixed-address-space.cpp
I am trying to build a Windows workspace to check what is wrong.
There was a problem hiding this comment.
I don't think the failing test is due to this patch. I was unable to reproduce issue on my local system and so I took a closer look at the test - https://github.com/intel/llvm-test-suite/blob/intel/SYCL/Printf/mixed-address-space.cpp. The kernel here isn't capturing anything. So I don't know why this patch would affect it (since the change in this patch only applies to function pointer kernel arguments)
There was a problem hiding this comment.
since April 2020 (or whenever this commit was pulled down to DPCPP project https://reviews.llvm.org/D77119), the address space has been 0, which is why there was a regression downstream after https://reviews.llvm.org/D119045.
Thanks, this makes me a bit more confident about what the patch does.
There was a problem hiding this comment.
Being honest I'm not sure what is the correct address space for function pointers in kernel arguments. It would be great to get feedback from @AlexeySachkov .
At SYCL level, we don't plan to have function pointers as kernel arguments
|
Is there a way to just restart failing test/pre-commit tests? I want to check if it is flaky |
You can sign in and restart testing (jenkins/precommit) |
|
@bader - the failing pre-commit tests are different from last time. I didn't change the PR before forcing a new run. So these new fails (and the old one which no longer fails) are flaky |
Commits 57fd86d (https://reviews.llvm.org/D77119) and
4eaf584 (https://reviews.llvm.org/D111566) set
address space of function pointers unconditionally to program
address space.
A follow-up commit ed5b42b (https://reviews.llvm.org/D119045)
modified the code to retain explicitly specified address spaces
for function pointer. This introduced a regression in cases where
function pointers were captured as kernel arguments, since OpenCL
kernel generation sets global address space explicitly to all
pointers. This patch reverts the behavior for function pointers
captured as kernel arguments.
Signed-off-by: Elizabeth Andrews elizabeth.andrews@intel.com