Skip to content

[SYCL] Fix sycl-post-link when no split and symbols are requested. #1454

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 3, 2020
Merged

[SYCL] Fix sycl-post-link when no split and symbols are requested. #1454

merged 3 commits into from
Apr 3, 2020

Conversation

kbobrovs
Copy link
Contributor

@kbobrovs kbobrovs commented Apr 2, 2020

Signed-off-by: Konstantin S Bobrovsky konstantin.s.bobrovsky@intel.com

@kbobrovs
[SYCL] Fix sycl-post-link when no split and symbols are requested.
67a5e37 
Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>
@kbobrovs kbobrovs requested a review from Fznamznon April 2, 2020 05:25
Fznamznon
Fznamznon reviewed Apr 2, 2020
View reviewed changes
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

Generally ok, but please fix tests.

llvm/test/tools/sycl-post-link/sym_but_no_split.ll Outdated
; table and a symbol file for an input module with two kernels when no code
; splitting is requested.
;
; RUN: sycl-post-link -symbols -spec-const=rt -S %s -o %T/files.table
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose tests are failing in pre-commit due conflict of test files.

Suggested change
; RUN: sycl-post-link -symbols -spec-const=rt -S %s -o %T/files.table
; RUN: sycl-post-link -symbols -spec-const=rt -S %s -o %T/%t.files.table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the hint, BTW )

llvm/tools/sycl-post-link/sycl-post-link.cpp Outdated
Comment on lines 124 to 125
// Describes scope covered by each entry in the module-kernel map populated by
// the function below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, what if someone would come and made this comment invalid by adding new function after this enum...

Suggested change
// Describes scope covered by each entry in the module-kernel map populated by
// the function below.
// Describes scope covered by each entry in the module-kernel map populated by
// the collectKernelModuleMap function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

llvm/tools/sycl-post-link/sycl-post-link.cpp Outdated
Comment on lines 132 to 134
// Output parameter ResKernelModuleMap is a map containing groups of kernels
// with same values of the sycl-module-id attribute.
// The function fills ResKernelModuleMap using input module M.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rewrite this comment somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the comment has been invalid since addition of per-kernel split before my changes.
But OK, I'll go ahead and change the comment

llvm/tools/sycl-post-link/sycl-post-link.cpp
Comment on lines +149 to +151
error("no '" + Twine(ATTR_SYCL_MODULE_ID) +
"' attribute in kernel '" + F.getName() +
"', per-module split not possible");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we group kernels without this attribute into a separate module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, maybe. The answer would be 'yes' if there are practical scenarios where per-module splitting is requested on modules with functions w/o module-id attribute. This would be a topic for a separate PR, anyway.

Here I just protect from the existing problem that I noticed - silent dropping of kernels w/o module-id attribute in -split=source mode. What I can do now is add a TODO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. I thought about this case while implementing split, but I didn't come up with the viable case when it is possible to get such module containing kernels with and without module-id attribute.

@kbobrovs kbobrovs requested a review from bader as a code owner April 2, 2020 22:53
@kbobrovs
[SYCL] Implement review comments for PR intel#1454.
27b88f5 
Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>
@kbobrovs kbobrovs force-pushed the private/kbobrovs-sycl branch from 01c8bd3 to 27b88f5 Compare April 2, 2020 22:57
@kbobrovs kbobrovs requested a review from Fznamznon April 3, 2020 04:52
Fznamznon
Fznamznon previously approved these changes Apr 3, 2020
View reviewed changes
@kbobrovs @Fznamznon
Update llvm/tools/sycl-post-link/sycl-post-link.cpp
6f76fd8 
Co-Authored-By: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>
@kbobrovs kbobrovs requested a review from Fznamznon April 3, 2020 15:11
@bader bader merged commit 88144f7 into intel:sycl Apr 3, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 6, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/hide…
b5c2289 
…_private_api

* origin/sycl: (614 commits)
  [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466)
  [SYCL][USM] Remove vestigial dead code (intel#1474)
  [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451)
  [Driver][SYCL] Emit an error if c compilation is forced (intel#1438)
  [SYCL] Fix sycl-post-link when no split and symbols are requested. (intel#1454)
  [SYCL] Change priority of devices in default_selector (intel#1264)
  [CI] Update CODEOWNERS matching rules order (intel#1468)
  [SYCL] Share PFWG lambda object through shared memory (intel#1455)
  [CI] Fix CODEOWNERS file syntax (intel#1464)
  [SYCL][CUDA] Fix active context when creating base event (intel#1447)
  [SYCL] Diagnose implicit declaration of kernel function type (intel#1450)
  [BuildBot] Modify configure script (intel#1421)
  [SYCL] Resolve min/max conflict (intel#1339)
  [CI][BuildBot] Fix configure parameter to turn on/off assertions (intel#1449)
  [SYCL] XFAIL LIT test due to duplicate diagnostic
  [SYCL] Remove explicit sycl_device attribute requirement
  Apply more suggestions
  Apply suggestions
  Translate new set of Intel FPGA Loop Controls
  Translate Intel FPGA force_pow2_depth memory attribute
  ...
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 15, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/basi…
961afa4 
…c_abi_checks

* origin/sycl: (625 commits)
  [SYCL][Test] Disable spec_const_redefine.cpp on all devices but HOST (intel#1488)
  [SYCL] Only export public API (intel#1456)
  [SYCL][CUDA] Fix selected_binary argument in piextDeviceSelectBinary (intel#1475)
  [SYCL] Enable LIT testing with CUDA BE (intel#1458)
  [SYCL] Fix float to half-type conversion (intel#1395)
  [NFC] Cleanup unneded macro from builtins implementation (intel#1445)
  Enable cfg-printer LLVM lit tests only if LLVM linked statically (intel#1479)
  [SYCL][NFC] Reflect the "allowlist" renaming in the code (intel#1480)
  [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466)
  [SYCL][USM] Remove vestigial dead code (intel#1474)
  [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451)
  [Driver][SYCL] Emit an error if c compilation is forced (intel#1438)
  [SYCL] Fix sycl-post-link when no split and symbols are requested. (intel#1454)
  [SYCL] Change priority of devices in default_selector (intel#1264)
  [CI] Update CODEOWNERS matching rules order (intel#1468)
  [SYCL] Share PFWG lambda object through shared memory (intel#1455)
  [CI] Fix CODEOWNERS file syntax (intel#1464)
  [SYCL][CUDA] Fix active context when creating base event (intel#1447)
  [SYCL] Diagnose implicit declaration of kernel function type (intel#1450)
  [BuildBot] Modify configure script (intel#1421)
  ...
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Fznamznon Fznamznon Fznamznon approved these changes

@bader bader Awaiting requested review from bader

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kbobrovs @Fznamznon @bader