Skip to content

[sycl-post-link] Enable ESIMD-specific lowering #3195

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 15, 2021

Conversation

DenisBakhvalov
Copy link
Contributor

This is the first patch in the effort of moving ESIMD-specific
lowering passes from clang FE (BackendUtils.cpp) to the
sycl-post-link tool after SYCL-ESIMD splitting.

This is the first patch in the effort of moving ESIMD-specific
lowering passes from clang FE (BackendUtils.cpp) to the
sycl-post-link tool after SYCL-ESIMD splitting.
@DenisBakhvalov
Copy link
Contributor Author

A couple of comments explaining certain implementation details:

  • Option set in sycl-post-link is inherited from the opt tool.
  • I put the ESIMD lowering under -lower-esimd option, to ease the transition of the lowering from FE into sycl-post-link.

This patch doesn't have an immediate impact. In a later patch, I will add the corresponding code in the clang driver.

Comment on lines +688 to +689
// When ESIMD code was separated from the regular SYCL code,
// we can safely process ESIMD part.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably an architecture question/concern: why can't we leave a single optimization pipeline which is executed before sycl-post-link invocation?

I understand that IGC VC backend most likely expects LLVM IR in some particular form to do its job right, but can't it be improved? It seems a bit strange to me that we have to maintain two optimization pipelines in two different places. Moreover, I guess that the first pipeline before sycl-post-link will still be executed to optimize regular non-ESIMD code, right?

Note: I'm okay if we have to launch a few more passes in order to launch some ESIMD-specific ones, but I would like to have a single optimization pipeline (potentially customized compared to the upstream one) for general-purpose optimizations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexeySachkov, yes, those are ESIMD-specific transformations that should be applied to ESIMD code only, otherwise, it will break regular SYCL code. So, it should be done after splitting. Or we should clone the code that is shared between ESIMD and regular SYCL kernels and then do the ESIMD lowering. Splitting first and then transform, eliminates the need to clone shared code, so I guess this is definitely an advantage.

Yes, we have general-purposes clean-up passes after ESIMD lowering. So, yes, we have 2 sets of general-purpose passes running on ESIMD code. I believe we have to have the second set after ESIMD lowering. @cmc-rep can confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

those are ESIMD-specific transformations that should be applied to ESIMD code only, otherwise, it will break regular SYCL code.

I'm not against ESIMD-specific transformation, I'm concerned about additional optimization passes we launch in sycl-post-link

Yes, we have general-purposes clean-up passes after ESIMD lowering. So, yes, we have 2 sets of general-purpose passes running on ESIMD code. I believe we have to have the second set after ESIMD lowering. @cmc-rep can confirm.

To me it seems better if those post-processing is moved into vc backend as pre-processing passes, because this is a thing specific to a single backend, not even to a language constructs.

Comment on lines +102 to +127
static cl::opt<bool>
OptLevelO0("O0", cl::desc("Optimization level 0. Similar to clang -O0"),
cl::cat(PostLinkCat));

static cl::opt<bool>
OptLevelO1("O1", cl::desc("Optimization level 1. Similar to clang -O1"),
cl::cat(PostLinkCat));

static cl::opt<bool>
OptLevelO2("O2", cl::desc("Optimization level 2. Similar to clang -O2"),
cl::cat(PostLinkCat));

static cl::opt<bool> OptLevelOs(
"Os",
cl::desc(
"Like -O2 with extra optimizations for size. Similar to clang -Os"),
cl::cat(PostLinkCat));

static cl::opt<bool> OptLevelOz(
"Oz",
cl::desc("Like -Os but reduces code size further. Similar to clang -Oz"),
cl::cat(PostLinkCat));

static cl::opt<bool>
OptLevelO3("O3", cl::desc("Optimization level 3. Similar to clang -O3"),
cl::cat(PostLinkCat));
Copy link
Contributor

Choose a reason for hiding this comment

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

How these are expected to be used? Will they be propagated to the tool based on the same options passed to clang driver?

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, I will submit the patch for that later and put the link here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the driver portion that will pass the optimization level to sycl-post-link: #3207.

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader requested a review from AlexeySachkov February 12, 2021 06:57
@DenisBakhvalov
Copy link
Contributor Author

@AlexeySachkov please review/approve.

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

I'm still concerned about having additional optimization pipeline in sycl-post-link, but this concern shouldn't block the commit

@bader bader merged commit 3d2adc7 into intel:sycl Feb 15, 2021
@bader
Copy link
Contributor

bader commented Feb 15, 2021

@DenisBakhvalov, this patch breaks the build with shared libraries. https://github.com/intel/llvm/runs/1903069847?check_suite_focus=true
Looks like a missing dependency. Could you fix it ASAP, pelase?

@DenisBakhvalov
Copy link
Contributor Author

@DenisBakhvalov, this patch breaks the build with shared libraries. https://github.com/intel/llvm/runs/1903069847?check_suite_focus=true
Looks like a missing dependency. Could you fix it ASAP, pelase?

@bader, Taking a look...

@DenisBakhvalov
Copy link
Contributor Author

@DenisBakhvalov, this patch breaks the build with shared libraries. https://github.com/intel/llvm/runs/1903069847?check_suite_focus=true
Looks like a missing dependency. Could you fix it ASAP, pelase?

@bader, Taking a look...

Opened #3221.

zahiraam pushed a commit to zahiraam/llvm-1 that referenced this pull request Feb 16, 2021
This is the first patch in the effort of moving ESIMD-specific
lowering passes from clang FE (BackendUtils.cpp) to the
sycl-post-link tool after SYCL-ESIMD splitting.

Co-authored-by: kbobrovs <[email protected]>
bader pushed a commit that referenced this pull request Feb 18, 2021
This is one of the patches for moving ESIMD passes from clang FE (BackendUtils.cpp) to sycl-post-link.
This patch is a continuation for #3195 and it passes the optimization level to the sycl-post-link.
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.

5 participants