Add kernel property to propagate compile options to backend by asudarsa · Pull Request #7373 · intel/llvm

asudarsa · 2022-11-11T13:31:20Z

There is a user requirement to provide ease of debuggability. Consider compilation flow:
clang++ -O0 -fsycl -c -g test.cpp -o test.o
clang++ -fsycl test.o
The resulting a.out fat binary is sent to the runtime without any associated flag. The -O0 flag used using first stage of compilation is lost. This causes user pain during debug sessions and has been reported as such.
Change in this PR does the following:

Introduces a new SYCL kernel-level attribute sycl-device-compile-optlevel with the optlevel as the associated value.
This option is added during front-end code generation.
During sycl-post-link stage, the modules are split based on opt-level.
A new entry in the props file to record the opt-level is provided.
SYCL runtime then interprets this opt-level and passes the appropriate flag to the backend.
Signed-off-by: Arvind Sudarsanam arvind.sudarsanam@intel.com

clang/include/clang/Basic/Attr.td

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

AlexeySachkov · 2022-11-11T13:54:49Z

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

What if we have functions with different options metadata in the same module?

Module splitting code based on opt level has been added.

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

clang/include/clang/Basic/Attr.td

clang/lib/CodeGen/CodeGenFunction.cpp

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa · 2022-12-21T20:49:32Z

I will be adding tests and updating the documentation file before EOD tomorrow, if not sooner. Thanks

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa · 2022-12-22T15:58:27Z

The test failure seems sporadic. Will check.

AlexeySachkov

I would suggest that we finish and agree on the design doc first to avoid doing implementation steps which may be reverted/revisited later.

I agree that this feature needs a design doc, but the proposed one does not seem to be detailed enough

AlexeySachkov · 2022-12-22T15:29:12Z

clang/lib/CodeGen/CodeGenFunction.cpp

 // This coordinates the per-function state used while generating code.
 //
 //===----------------------------------------------------------------------===//
-


We should avoid doing unrelated changes

AlexeySachkov · 2022-12-22T15:43:53Z

sycl/doc/design/PropagateCompilerFlagsToLinkerAndRuntime.md

+
+## Background
+
+When building an application with several source and object files, it should be possible to specify the optimization parameters individually for each source file/object file (for each invocation of the DPCPP compiler). The linker should pass the original optimization options (e.g. -O0 or -O2) used when building an object file to the device backend compiler (IGC compiler). This will improve the debugging experience by selectively disabling/enabling optimizations for each source file, and therefore achieving better debuggability and better performance as needed.


I don't think that we should mention IGC here, because we have several device compilers

Agreed. I will remove it.

AlexeySachkov · 2022-12-22T15:44:41Z

sycl/doc/design/PropagateCompilerFlagsToLinkerAndRuntime.md

+Here is an example that demonstrates this pain point:
+
+```
+icx -c -fsycl test1.c -o test1


Neither icx nor icpx exist in this repo, I suggest that we use clang++

Oops..That's correct. I will change it. Good catch. Thanks

AlexeySachkov · 2022-12-22T16:11:57Z

sycl/doc/design/PropagateCompilerFlagsToLinkerAndRuntime.md

+In order to support module-level debuggability, the user will compile different module files with different levels of optimization. These optimization levels must be preserved and made use of during every stage of compilation. Following are the requirements for this feature.
+- If the user specifies '-Ox' as a front-end compile option for a particular module, this option must be preserved during compilation, linking, AOT compilation as well as JIT compilation.
+- If the user specifies '-Ox' option as a front-end linker option, this option will override any front-end compile options and the linker option will be preserved during AOT and JIT compilation.
+- If the user specifies '-O0' option, we need to pass '-cl-opt-disable' to AOT and JIT compilation stages.


I think we should either remove this or rewrite this. Level Zero does not support -cl-opt-disable flag. I think our aim is to achieve a mapping between front-end optimizations options and backend optimizations options, which is as close as possible and respects original options passed to compilation of each translation unit.

cl-opt-disable is an option being passed to the backend compiler. I don't think we need to consider whether it's L0 or OpenCL.

Level Zero spec does not list -cl-opt-disable as a supported option. It is not clear to me if zeModuleCreate must return an error for unsupported options and which one it would be, but I would prefer if we don't pass there options which are not considered supported.

AlexeySachkov · 2022-12-22T16:13:29Z

sycl/doc/design/PropagateCompilerFlagsToLinkerAndRuntime.md

+
+Following are changes required in various stages of the compilation pipeline:
+- Front-end code generation: For each SYCL kernel, identify the compilation option. Add an appropriate attribute to that kernel. Name of that attribute is 'sycl-device-compile-optlevel'.
+- During the llvm-link stage, all modules are linked into a single module. This is an existing behavior.


I suggest we only document changes to the existing implementation

AlexeySachkov · 2022-12-22T16:57:04Z

clang/lib/CodeGen/CodeGenFunction.cpp

+      SYCLDeviceCompileOptLevel = CGM.getCodeGenOpts().OptimizationLevel;
+    }
+    if (SYCLDeviceCompileOptLevel >= 0)
+      Fn->addFnAttr("sycl-device-compile-optlevel", std::to_string(SYCLDeviceCompileOptLevel));


LIT test is missing for this change

I added the check inside the sycl test to check if he attribute is being added. Do you want me to split it up? Thanks

Usually, we have unit-tests for each component. The test in sycl/ subdirectory is ok, but that is an integration test and if it fails, it won't be immediately clear why, because the failure could be in any component throughout the stack.

Unit-tests which are limited to a single component will allow to highlight issues more precisely

AlexeySachkov · 2022-12-22T16:57:21Z

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

+
+  auto OptLevel = MD.getOptLevel();
+  if (OptLevel >= 0)
+    PropSet[PropSetRegTy::SYCL_MISC_PROP].insert({"OptLevel", OptLevel});


LIT test is missing for this change

I thought I am covering this in sycl-opt-level.cpp. Can you please take a look? Thanks

AlexeySachkov · 2022-12-22T16:57:46Z

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

+                                          EmitOnlyKernelsAsEntryPoints);
+      SplitByOptLevel |= OptLevelSplitter->remainingSplits() > 1;
+      while (OptLevelSplitter->hasMoreSplits()) {
+        TopLevelModules.emplace_back(OptLevelSplitter->nextSplit());


LIT tests for new split dimension are missing

I am currently exploring the way to add LIT tests that include multiple source files. I will add this test soon. is that going to be a blocker o get this checked in? Thanks

AlexeySachkov · 2022-12-22T16:58:57Z

sycl/source/detail/program_manager/program_manager.cpp

+  int OptLevel = Prop ? DeviceBinaryProperty(Prop).asUint32() : -1;
+  std::string OptLevelStr = "";
+  // Currently, we do not do anything for other opt levels
+  // TODO: Figure out a way to send some info across for other opt levels.


Note: optimizations flags are different for each backend.

FYI:

Level Zero supported flags

OpenCL supported flags

AlexeySachkov · 2022-12-22T17:00:05Z

sycl/source/detail/program_manager/program_manager.cpp

+  if (!OptLevelStr.empty()) {
+    if (!CompileOpts.empty())
+      CompileOpts += " ";
+    CompileOpts += OptLevelStr;


Unit-tests are missing for RT changes

premanandrao · 2023-01-03T16:23:43Z

clang/lib/CodeGen/CodeGenFunction.cpp

+    int SYCLDeviceCompileOptLevel = -1;
+    switch (CGM.getCodeGenOpts().OptimizationLevel) {
+    default:
+      llvm_unreachable("Invalid optimization level!");
+    case 0:
+    case 1:
+    case 2:
+    case 3:
+      SYCLDeviceCompileOptLevel = CGM.getCodeGenOpts().OptimizationLevel;
+    }


Suggested change

int SYCLDeviceCompileOptLevel = -1;

switch (CGM.getCodeGenOpts().OptimizationLevel) {

default:

llvm_unreachable("Invalid optimization level!");

case 0:

case 1:

case 2:

case 3:

SYCLDeviceCompileOptLevel = CGM.getCodeGenOpts().OptimizationLevel;

}

int SYCLDeviceCompileOptLevel = CGM.getCodeGenOpts().OptimizationLevel;

if (SYCLDeviceCompileOptLevel < 0) ...;

Why not (something like) this instead? I am trying to understand the benefit of checking the 0,1,2,3 values in the switch.

bader · 2023-01-27T03:20:49Z

@asudarsa, what is status of PR?

bader · 2023-02-24T17:25:16Z

@asudarsa, what is status of PR?

Ping.

bader

Please, resolve merge conflicts.

…ler (#8763) This change is a second attempt to add this support. An earlier attempt was here: #7373 In this change, following changes have been made: 1. clang changes to add a new function attribute: sycl-optlevel 2. sycl-post-link changes to process this attribute, split modules based on optimization level, and add a new property named 'optLevel' to SYCL/misc properties' property set. 3. SYCL runtime and plugin changes to access this device image property and propagate a backend specific optimization flag to the backend compiler. 4. Documentation 5. 2 unit tests and 1 e2e test --------- Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

AlexeySachkov · 2023-04-20T13:15:33Z

Superseded by #8763

asudarsa requested review from a team as code owners November 11, 2022 13:31

asudarsa requested a review from KseniyaTikhomirova November 11, 2022 13:31

asudarsa marked this pull request as draft November 11, 2022 13:31

AlexeySachkov reviewed Nov 11, 2022

View reviewed changes

asudarsa added 3 commits December 13, 2022 08:46

Add kernel property to propagate compile options to backend

4d58f01

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

second stage of implementation

d73a1da

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Fix merge conficts

54d0eb4

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa force-pushed the asudarsa/propagate_compiler_opt_flags branch from 779ac48 to 54d0eb4 Compare December 21, 2022 19:38

asudarsa temporarily deployed to aws December 21, 2022 19:42 — with GitHub Actions Inactive

minor corrections

f4eefc5

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa temporarily deployed to aws December 21, 2022 20:58 — with GitHub Actions Inactive

Change default opt level from O2 to unknown

8c8e0e1

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa temporarily deployed to aws December 21, 2022 22:52 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws December 21, 2022 23:25 — with GitHub Actions Inactive

Fix more test failures

a997862

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa temporarily deployed to aws December 22, 2022 02:07 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws December 22, 2022 02:42 — with GitHub Actions Inactive

Fixing doc issue

fad0731

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa temporarily deployed to aws December 22, 2022 03:12 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws December 22, 2022 03:43 — with GitHub Actions Inactive

Add LIT test

4ea7a9c

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa marked this pull request as ready for review December 22, 2022 03:59

asudarsa requested review from a team as code owners December 22, 2022 03:59

asudarsa temporarily deployed to aws December 22, 2022 04:21 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws December 22, 2022 04:51 — with GitHub Actions Inactive

AlexeySachkov suggested changes Dec 22, 2022

View reviewed changes

premanandrao reviewed Jan 3, 2023

View reviewed changes

bader requested changes Mar 24, 2023

View reviewed changes

asudarsa mentioned this pull request Mar 24, 2023

[SYCL] Add support to propagate compile flags to device backend compiler #8763

Merged

AlexeySachkov closed this Apr 20, 2023


		## Background

		When building an application with several source and object files, it should be possible to specify the optimization parameters individually for each source file/object file (for each invocation of the DPCPP compiler). The linker should pass the original optimization options (e.g. -O0 or -O2) used when building an object file to the device backend compiler (IGC compiler). This will improve the debugging experience by selectively disabling/enabling optimizations for each source file, and therefore achieving better debuggability and better performance as needed.

Conversation

asudarsa commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asudarsa commented Dec 21, 2022

Uh oh!

asudarsa commented Dec 22, 2022

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader commented Jan 27, 2023

Uh oh!

bader commented Feb 24, 2023

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asudarsa commented Nov 11, 2022 •

edited

Loading