Skip to content

Conversation

AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Oct 4, 2021

Introduced new sycl-post-link option -emit-only-kernels-as-entry-points,
which allows to stop treating SYCL_EXTERNAL functions as entry points.

Updated clang driver to pass the new option for spir64_fpga target, because
on FPGA, there are no use cases for preserving SYCL_EXTERNAL functions
as separate entries in device images if they are not referenced and it only
causes compilation slowdowns due to increased amount of device code.

Added a flag `-emit-non-kernel-entry-points`, which is on by default and
can be turned off to disable treating `SYCL_EXTERNAL` functions with
`sycl-module-id` attribute as entry points.
AGindinson
AGindinson previously approved these changes Oct 4, 2021
@@ -29,6 +29,22 @@
// CHK-RANGE-ROUNDING: clang{{.*}} "-fsycl-is-device"{{.*}} "-fsycl-disable-range-rounding"
// CHK-RANGE-ROUNDING: clang{{.*}} "-fsycl-disable-range-rounding"{{.*}} "-fsycl-is-host"

/// FPGA target implies -emit-non-kernel-entry-points=0 in sycl-post-link
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to add a test case which checks that the default for other targets if no option

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's already being done on line 39. Nevertheless, if the default behavior of sycl-post-link gets changed, these checks will have to be adjusted accordingly.

@@ -175,6 +175,13 @@ static cl::opt<bool> EmitExportedSymbols{"emit-exported-symbols",
cl::desc("emit exported symbols"),
cl::cat(PostLinkCat)};

static cl::opt<bool> EmitNonKernelEntryPoints{
"emit-non-kernel-entry-points",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to reverse the semantic of the option, as "normal" behavior is to treat SYCL external functions just like kernels. "emit-only-kernel-entries" with default 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense, applied in 4cdb830

Copy link
Contributor

@AGindinson AGindinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@AlexeySachkov AlexeySachkov requested a review from kbobrovs October 5, 2021 13:22
AlexeySachkov added a commit to AlexeySachkov/llvm-test-suite that referenced this pull request Oct 5, 2021
The test failed in intel/llvm#4693 and intel/llvm#4649. I wasn't able to
reproduce the failure locally and restart made the issue dissappear.

Therefore, disabled the test until the flakiness is analyzed/root
caused.
@AlexeySachkov
Copy link
Contributor Author

I don't believe that the failure in Jenkins/Precommit is caused by this patch and I've seen it before in another PR as well. I've submitted intel/llvm-test-suite#497 to temporarily disable the flaky test and I'm waiting for Jenkins/Precommit restart here to double-check that the issue is indeed flaky and not caused by this PR.

@AlexeySachkov AlexeySachkov changed the title Treat only SYCL kernels as entry points for FPGA target [SYCL] Treat only SYCL kernels as entry points for FPGA target Oct 5, 2021
@AlexeySachkov
Copy link
Contributor Author

Restarted test passed, merging

@AlexeySachkov AlexeySachkov merged commit 41676bc into intel:sycl Oct 6, 2021
AlexeySachkov added a commit to AlexeySachkov/llvm that referenced this pull request Oct 7, 2021
Returned back small piece of code which was accidentally reverted
in intel#4693
AlexeySachkov added a commit that referenced this pull request Oct 8, 2021
Returned back small piece of code which was accidentally reverted
in #4693
@AlexeySachkov AlexeySachkov deleted the private/asachkov/add-emit-non-kernel-entry-points-flag branch May 22, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants