[SYCL] Update vtable emission logic for device code #16932
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Comment on lines
+12
to
+13
| // RUN: %{build} %S/Inputs/call.cpp -o %t.out %helper-includes %O0 | ||
| // RUN: %{run} %t.out |
There was a problem hiding this comment.
Suggested change
| // RUN: %{build} %S/Inputs/call.cpp -o %t.out %helper-includes %O0 | |
| // RUN: %{run} %t.out | |
| // RUN: %{build} %S/Inputs/call.cpp -o %t2.out %helper-includes %O0 | |
| // RUN: %{run} %t2.out |
Comment on lines
+9
to
+10
| // RUN: %{build} %S/Inputs/call.cpp %S/Inputs/vf.cpp -o %t.out %helper-includes %O0 | ||
| // RUN: %{run} %t.out |
There was a problem hiding this comment.
Suggested change
| // RUN: %{build} %S/Inputs/call.cpp %S/Inputs/vf.cpp -o %t.out %helper-includes %O0 | |
| // RUN: %{run} %t.out | |
| // RUN: %{build} %S/Inputs/call.cpp %S/Inputs/vf.cpp -o %t2.out %helper-includes %O0 | |
| // RUN: %{run} %t2.out |
Comment on lines
+11
to
+12
| // RUN: %{build} %S/Inputs/vf.cpp -o %t.out %helper-includes %O0 | ||
| // RUN: %{run} %t.out |
There was a problem hiding this comment.
Suggested change
| // RUN: %{build} %S/Inputs/vf.cpp -o %t.out %helper-includes %O0 | |
| // RUN: %{run} %t.out | |
| // RUN: %{build} %S/Inputs/vf.cpp -o %t2.out %helper-includes %O0 | |
| // RUN: %{run} %t2.out |
| @@ -3283,6 +3283,18 @@ void CodeGenModule::SetFunctionAttributes(GlobalDecl GD, llvm::Function *F, | |||
| if (const auto *A = FD->getAttr<SYCLUsesAspectsAttr>()) | |||
| applySYCLAspectsMD(A, getContext(), getLLVMContext(), F, | |||
| "sycl_used_aspects"); | |||
|
|
|||
| if (getLangOpts().SYCLIsDevice && | |||
There was a problem hiding this comment.
I am guessing we should not be adding this attribute in both SetFunctionAttributes and StartFunction. Have you checked if we're duplicating the attribute now? What happens when you remove the code in StartFunction
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There PR aims to fix these two issues:
calls-indirectlyattribute propagation doesn't always happen as necessary #15071:separate-vf-defs.cpp/separate-vf-defs-and-calls- The construction kernels were not being recognized bySYCLVirtualFunctionsAnalysisPass(i.e. attaching calls-indirectly to them). They were not being attached because the definitions for the vtables were not being emitted in those compilation units, andSYCLVirtualFunctionsAnalysisPasscan only correctly identifies construction kernels by analyzing the vtable definition referenced during object construction.separate-call.cpp- when sycl-post-link was splitting, the vtables were duplicated in the device images. This caused multiple definition errors between the duplicated vtables.1 is fixed by using
MarkVTableUsedon classes that have SYCLindirectly-callableattribute on one of their members, allowing the vtable definition to be generated. (Note: I would have liked to do thisMarkVTableUsedcall is inSemaSYCL::addSYCLAddIRAttributesFunctionAttrinstead ofSema::CheckCompletedCXXClassbut I was having an issue thatMarkVTableUseddid not updateVTablesUsedcorrectly because at that point the class wasn't recognized as virtual). With this, the vtable definition will be generated, but there is another problem - the functions referenced in the vtables did not have the relevantindirectly-callableattributes. These are usually attached by code gen if the function has a definition, but the definitions of the virtual functions do not necessarily need to be in the same compilation unit as the construction kernels. To fix this, a separate attribute attachment is added when code gen needs the pointer of some function. (Thesycl_used_aspectsmetadata is attached at the same time.)Since the fix for 1 emit vtables in every device module, we need to change the linkage type of the vtables during device compilation to
linkonce_odr, otherwise there could be multiple definition issues in the device link set. This change also happens to fix the issue of 2.