[sycl-post-link] Adds property listing exported functions #4626

steffenlarsen · 2021-09-23T11:14:37Z

These changes make sycl-post-link generate a list of exported functions as properties for each generated module. This is enabled through the -emit-exported-symbols option.

The properties follow the naming proposed in the Dynamic linking of device code documentation.

These changes make sycl-post-link generate a list of exported functions as properties for each generated module. This is enabled through the -emit-exported-symbols option. The properties follow the naming proposed in the "Dynamic linking of device code" documentation. Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

AlexeySachkov

sycl-post-link part LGTM. Just a few minor comments to consider (mostly notes about possible feature refactoring for myself)

llvm/test/tools/sycl-post-link/emit_exported_symbols.ll

AlexeySachkov · 2021-09-27T17:31:25Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+  StringRef KernelModuleName;
+  std::unique_ptr<Module> ModulePtr;
+};
+
 // Input parameter KernelModuleMap is a map containing groups of kernels with


Definitely Not asking to do such change in scope of this PR (to make it cleaner), but it seems to me that this comment (and KernelModuleMap name as well) has been outdated for some time now, because this map contains more than just kernels now

AlexeySachkov · 2021-09-27T17:50:30Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+    ImagePropSaveInfo ImgPSInfo = {true,
+                                   DoSpecConst,
+                                   SetSpecConstAtRT,
+                                   SpecConstsMet,
+                                   EmitKernelParamInfo,
+                                   EmitProgramMetadata,
+                                   EmitExportedSymbols,
+                                   IsEsimd};


On the one hand, you can access EmitExportedSymbols directly within saveDeviceImageProperty so you don't need to modify ImagePropSaveInfo. But on the other hand, making a local copy of it reduces the amount of global variables used by saveDeviceImageProperty allowing us to refactor the origin of EmitExportedSymbols flag within ImagePropSaveInfo struct.

I'm fine with the suggested change, but it is still a bit weird to me that we wrap some data into a struct even if that data is already available in the target function where we need it.

The formatter went a bit wild here. I don't think I have changed the behavior.

I don't think I have changed the behavior.

Yeah, I'm not saying that you have changed it. My point was that you could avoid extending ImagePropSaveInfo structure at all

Ah, I see what you mean. I think for consistency it should do the same as the other options, but I agree it is a bit redundant.

AlexeySachkov · 2021-09-27T17:53:25Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

@@ -954,9 +986,10 @@ int main(int argc, char **argv) {
  bool DoSpecConst = SpecConstLower.getNumOccurrences() > 0;
  bool DoParamInfo = EmitKernelParamInfo.getNumOccurrences() > 0;
  bool DoProgMetadata = EmitProgramMetadata.getNumOccurrences() > 0;
+  bool DoExportedSyms = EmitExportedSymbols.getNumOccurrences() > 0;


I guess this is produced by a copy-pasting as well as almost all boolean opts above, but strictly speaking, you can pass false/0 to the option, which would mean that number of occurrences would be one, but corresponding EmitExportedSymbols flag would be evaluated as false, making the message emitted below on 1022 incorrect.

This is definitely a minor comment - we can leave it as-is, because the tool is not used directly by end-users anyway; or perform a refactoring in a separate PR for all boolean opts all together

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

AlexeySachkov

sycl-post-link still LGTM

smaslov-intel

pi.h changes LGTM

* sycl: (108 commits) [SYCL][XPTI] Revisit resource management strategy (intel#4494) [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681) [SYCL] Make kernel_bundle interop more conformant (intel#4672) [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667) [SYCL] Add support for std::byte to vec class (intel#4637) [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671) [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668) [SYCL] Exclude exported symbols from kernel bundles (intel#4660) Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659) [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639) [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657) [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629) [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663) [SYCL] Fix device code instrumentation (intel#4615) Remove myself as a code owner (intel#4653) [SYCL] Fix overwriting insert to sub_group_mask (intel#4656) [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641) [SYCL][Doc] Add device global extension spec (intel#4233) [sycl-post-link] Adds property listing exported functions (intel#4626) [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644) ...

* sycl: (107 commits) [SYCL][XPTI] Revisit resource management strategy (intel#4494) [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681) [SYCL] Make kernel_bundle interop more conformant (intel#4672) [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667) [SYCL] Add support for std::byte to vec class (intel#4637) [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671) [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668) [SYCL] Exclude exported symbols from kernel bundles (intel#4660) Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659) [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639) [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657) [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629) [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663) [SYCL] Fix device code instrumentation (intel#4615) Remove myself as a code owner (intel#4653) [SYCL] Fix overwriting insert to sub_group_mask (intel#4656) [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641) [SYCL][Doc] Add device global extension spec (intel#4233) [sycl-post-link] Adds property listing exported functions (intel#4626) [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644) ...

steffenlarsen requested a review from mlychkov September 23, 2021 11:14

steffenlarsen requested review from AGindinson, AlexeySachkov, hchilama, kbobrovs, mdtoguchi and smaslov-intel as code owners September 23, 2021 11:14

steffenlarsen added 2 commits September 23, 2021 14:18

Fix formatting

3cb7fd2

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Add new option to driver test

368a5bc

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

AGindinson previously approved these changes Sep 23, 2021

View reviewed changes

AlexeySachkov previously approved these changes Sep 27, 2021

View reviewed changes

steffenlarsen added 2 commits September 28, 2021 11:07

Remove table file checks

545d22f

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

Add implicit-check-not for NotExported

79da471

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen dismissed stale reviews from AlexeySachkov and AGindinson via 79da471 September 28, 2021 10:30

AlexeySachkov approved these changes Sep 28, 2021

View reviewed changes

smaslov-intel approved these changes Sep 28, 2021

View reviewed changes

AGindinson approved these changes Sep 28, 2021

View reviewed changes

AlexeySachkov merged commit ac706af into intel:sycl Sep 29, 2021

steffenlarsen deleted the steffen/sycl-post-link-exported-symbols branch December 6, 2023 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sycl-post-link] Adds property listing exported functions #4626

[sycl-post-link] Adds property listing exported functions #4626

steffenlarsen commented Sep 23, 2021

AlexeySachkov left a comment

AlexeySachkov Sep 27, 2021

AlexeySachkov Sep 27, 2021

steffenlarsen Sep 27, 2021

AlexeySachkov Sep 28, 2021

steffenlarsen Sep 28, 2021

AlexeySachkov Sep 27, 2021

AlexeySachkov left a comment

smaslov-intel left a comment

[sycl-post-link] Adds property listing exported functions #4626

[sycl-post-link] Adds property listing exported functions #4626

Conversation

steffenlarsen commented Sep 23, 2021

AlexeySachkov left a comment

Choose a reason for hiding this comment

AlexeySachkov Sep 27, 2021

Choose a reason for hiding this comment

AlexeySachkov Sep 27, 2021

Choose a reason for hiding this comment

steffenlarsen Sep 27, 2021

Choose a reason for hiding this comment

AlexeySachkov Sep 28, 2021

Choose a reason for hiding this comment

steffenlarsen Sep 28, 2021

Choose a reason for hiding this comment

AlexeySachkov Sep 27, 2021

Choose a reason for hiding this comment

AlexeySachkov left a comment

Choose a reason for hiding this comment

smaslov-intel left a comment

Choose a reason for hiding this comment