Skip to content

[SYCL][ESIMD][NewPM] Add ESIMDVerifierPass to new PM pipeline in CodeGen #5298

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.

Sign up for GitHub

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 4 commits into from
Feb 2, 2022
Merged

[SYCL][ESIMD][NewPM] Add ESIMDVerifierPass to new PM pipeline in CodeGen #5298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff1ffd0
[SYCL][ESIMD][NewPM] Add ESIMDVerifierPass to new PM pipeline in CodeGen
mlychkov Jan 13, 2022
988c7d9
Add testing for O0 opt level
mlychkov Jan 26, 2022
48909ac
Merge remote-tracking branch 'origin/sycl' into newPM_add_esimd_vrfy_…
mlychkov Jan 27, 2022
c552764
Merge remote-tracking branch 'origin/sycl' into newPM_add_esimd_vrfy_…
mlychkov Jan 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions clang/lib/CodeGen/BackendUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1384,6 +1384,12 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
// configure the pipeline.
OptimizationLevel Level = mapToLevel(CodeGenOpts);

if (LangOpts.SYCLIsDevice)
Copy link
Contributor

Choose a reason for hiding this comment

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

ESIMD verification should ideally run even if LLVM passes are disabled, as this is logically part of FE semantic analysis.
@sndmitriev - what do you think?

Copy link
Contributor

@AlexeySachkov AlexeySachkov Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ESIMD verification should ideally run even if LLVM passes are disabled, as this is logically part of FE semantic analysis.
@sndmitriev - what do you think?

Please note that disable-llvm-passes disables even such passes as AlwaysInline, which are required to preserve the semantics of always_inline attribute, i.e. the flag literally says disable llvm passes, not optimization or transformation passes, but llvm passes.

My understanding is that the flag is not intended to be used by end users unless they know what they are doing and therefore, I would prefer to leave ESIMD verification disabled if -disable-llvm-passes is present.

@bader, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Alexey. -disable-llvm-passes is supposed to disable all LLVM passes with no exceptions.
I think what Konst has in mind is that ESIMD verifier pass should run even with -O0.

Copy link
Contributor

Choose a reason for hiding this comment

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

ESIMD verification should ideally run even if LLVM passes are disabled, as this is logically part of FE semantic analysis.

Can we move the logic of this pass to FE Sema then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbobrovs @sndmitriev Could you please provide any comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for delay.
Unfortunately, we can't move this to Sema as it was identified as very time-consuming task by the FE team, so we went with the simpler solution.

I think what Konst has in mind is that ESIMD verifier pass should run even with -O0.

Yes, exactly.

PB.registerPipelineStartEPCallback(
[](ModulePassManager &MPM, OptimizationLevel Level) {
MPM.addPass(ESIMDVerifierPass());
});

bool IsThinLTO = CodeGenOpts.PrepareForThinLTO;
bool IsLTO = CodeGenOpts.PrepareForLTO;

Expand Down
5 changes: 4 additions & 1 deletion sycl/test/esimd/esimd_verify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
// RUN: not %clangxx -fsycl -fsycl-device-only -S %s -o %t 2>&1 | FileCheck %s
// RUN: not %clangxx -fsycl -fsycl-device-only -flegacy-pass-manager -S %s -o /dev/null 2>&1 | FileCheck %s
// RUN: not %clangxx -fsycl -fsycl-device-only -fno-legacy-pass-manager -S %s -o /dev/null 2>&1 | FileCheck %s
// RUN: not %clangxx -fsycl -fsycl-device-only -flegacy-pass-manager -O0 -S %s -o /dev/null 2>&1 | FileCheck %s
// RUN: not %clangxx -fsycl -fsycl-device-only -fno-legacy-pass-manager -O0 -S %s -o /dev/null 2>&1 | FileCheck %s

#include <sycl/ext/intel/experimental/esimd.hpp>

Expand Down