[SYCL][NATIVECPU] Enable source-based code coverage in Native CPU #15073
Conversation
uwedolinsky
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
uwedolinsky
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uwedolinsky
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
to process the kernel (in NativeCPU). This also requires
setting the debug location temporarily to the kernel arg
location so that the expected file is picked up by the
SourceLocToDebugLoc function used by SYCLOptReportHandler.
uwedolinsky
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Co-authored-by: Michael Toguchi <[email protected]>
uwedolinsky
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Co-authored-by: Michael Toguchi <[email protected]>
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
There was a problem hiding this comment.
It isn't clear to me what the FE changes are doing in this patch. Please add a FE test if there is functionality that can be tested with these changes. It looks like you are only testing driver changes here. If that is the only functionality that can be tested at the moment, please remove FE changes from this PR and add them when it can be tested.
| @@ -1786,6 +1786,9 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn, | |||
| if (SyclOptReport.HasOptReportInfo(FD)) { | |||
| llvm::OptimizationRemarkEmitter ORE(Fn); | |||
| for (auto ORI : llvm::enumerate(SyclOptReport.GetInfo(FD))) { | |||
| // Temporarily apply arg location to ensure SourceLocToDebugLoc | |||
| // picks up the expected file. | |||
| ApplyDebugLocation tmp_raii(*this, ORI.value().KernelArgLoc); | |||
There was a problem hiding this comment.
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.
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.
| return CompoundStmt::Create(SemaSYCLRef.getASTContext(), BodyStmts, | ||
| FPOptionsOverride(), {}, {}); | ||
| FPOptionsOverride(), LL, LR); |
There was a problem hiding this comment.
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.
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.
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.
@elizabethandrews @tahonermann do you have updates on this? Thank you
There was a problem hiding this comment.
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.
@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.
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
@elizabethandrews I have updated the PR description with more details and explanations about the changes, which are all covered by existing tests. Please let me know if anything needs further clarification. Thanks |
@uwedolinsky FE changes need FE tests. The tests you added are to test driver changes and an E2E test verifying FE changes. I do not think this is sufficient. IIUC you patch should have changed source locations in AST. This should be tested via a FE AST test. |
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uwedolinsky
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
@elizabethandrews I've added a new AST test to check that the compound statement of the kernel body has a valid source location. Is that acceptable? |
Yes. Thank you |
|
@intel/dpcpp-tools-reviewers please could you review? Thank you |
|
@intel/llvm-gatekeepers this PR looks ready to merge. The approval from @intel/dpcpp-tools-reviewers seems no longer required because I reverted a modifcation to a file owned by this group. Please could you double-check? Thank you |
Supports clang's source-based code coverage to enable code coverage testing of SYCL applications via the Native CPU SYCL target.
Clang's
-fprofile-instr-generate -fcoverage-mappingoptions can now be used with thenative_cpuSYCL target to compile/instrument host and device code, enabling 'llvm-cov' to render a coverage report after running the SYCL application (see also updated documentation in this PR).Subsequent PRs will enable in NativeCPU more of the currently unsupported options for device compilation, also for performance profiling.
Details and explanations for the changes in this PR:
This PR tests coverage options on the existing NativeCPU vector-add test by adding an additional invocation with previously disabled options
-fprofile-instr-generate -fcoverage-mapping -mllvm -system-headers-coverage. Enabling these options on device code caused an assert in the upstream clang profiling code generation tools due to the invalid source location on the AST for the implicitly generated kernel body, specifically the compound statement containing the kernel body. This PR honors this upstream clang assert by replacing the invalid source location in the compound statement with the source location of the kernel body. Using this now valid source location maintains the location (of the kernel caller function) currently tested by non-upstream-llvm lit testCodeGenSYCL/debug-info-srcpos-kernel.cpp, but exposed an issue that led to a change in behavior in non-llvm-upstream lit testSemaSYCL/kernel-arg-opt-report.cpp, which was due to the previously invalid source location causing the compiler to skip code to set the current location. To restore the original behavior of this test (checking for the location of the kernel functor, as opposed to the kernel caller function) this PR temporarily (and only for the purpose of generating the report) sets the current location to the location of the kernel argument using the upstream clang utility clang::CodeGen::ApplyDebugLocation.