-
Notifications
You must be signed in to change notification settings - Fork 744
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
[SYCL][NATIVECPU] Enable source-based code coverage in Native CPU #15073
Changes from 24 commits
4147842
e72e647
ef55ccb
2755480
f28c4fb
b02ef1e
4e6664d
593a981
149a352
f56c4d4
896ca81
0ec7691
55d31e7
b90ea9b
094e10a
f14c4f2
7acb09f
da794ff
d6dff0e
637caec
cb35294
6b72582
ad356fb
ea90f2f
c25bad1
82e4f64
a0d2d55
49fa00f
a20329c
e8f776c
d3558b7
ae41bfa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3602,8 +3602,12 @@ class SyclKernelBodyCreator : public SyclKernelFieldHandler { | |
BodyStmts.insert(BodyStmts.end(), FinalizeStmts.begin(), | ||
FinalizeStmts.end()); | ||
|
||
const auto *LDcl = NewBody; | ||
elizabethandrews marked this conversation as resolved.
Show resolved
Hide resolved
|
||
SourceLocation LL = LDcl ? LDcl->getBeginLoc() : SourceLocation(); | ||
SourceLocation LR = LDcl ? LDcl->getEndLoc() : SourceLocation(); | ||
|
||
return CompoundStmt::Create(SemaSYCLRef.getASTContext(), BodyStmts, | ||
FPOptionsOverride(), {}, {}); | ||
FPOptionsOverride(), LL, LR); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this actually doing? This is supposed to correspond to location in source code IIUC which of course doesn't exist for compiler generated code. So what location is being picked up here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is picking up the source location from the kernel body - so the compound statement uses the same location as the kernel body. You are right, this is a compiler-generated function, but it still needs a valid source location to ensure any tools processing the clang AST and requiring that source location to be valid to succeed. Other compiler-generated function like constructors also have valid locations even though they don't exist in source code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this sounds reasonable but I would like @tahonermann to also weigh in here. IIRC the work we re currently implementing for upstream clang (which will replace a lot of this code eventually) does not set SourceLocations as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elizabethandrews @tahonermann do you have updates on this? Thank you There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I somehow missed @elizabethandrews's earlier request for me to weigh in on this. I think this is reasonable. I'm not sure there is clear guidance for cases like this where a function definition is composed from both generated and written code. There might be cases in the new upstream implementation that will need to have source locations added. I think I generally opted for generated code to have builtin/unknown source locations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tahonermann I was trying to set the location similar to what clang does with implicitly generated constructors which also get a valid source location (the location of the struct name). The main aim of this PR is just to avoid asserts on invalid source locations in the clang AST when generating profiling/coverage code (Tested by the new invocation added to sycl/test/native_cpu/vector-add.cpp. |
||
} | ||
|
||
void annotateHierarchicalParallelismAPICalls() { | ||
|
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.
Please use Pascal Case for naming variables. `TempRAII is an option.
It also isn't clear to me where this is used.
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.
I've changed the variable name, hope it's ok.
This variable isn't (directly) used by the subsequent code in this function - it's just a RAII variable to temporarily set the argument location to maintain the current behavior of this non-llvm-upstream lit test. I have updated the PR description with more details. Please let me know if more clarification is needed. Thanks.