Skip to content

[ESIMD] Don't add inline hints when compiling for -O0 #3422

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

Closed
wants to merge 1 commit into from
Closed

[ESIMD] Don't add inline hints when compiling for -O0 #3422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion llvm/include/llvm/SYCLLowerIR/LowerESIMD.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,15 @@ namespace llvm {
class SYCLLowerESIMDPass : public PassInfoMixin<SYCLLowerESIMDPass> {
public:
PreservedAnalyses run(Module &M, ModuleAnalysisManager &);
SYCLLowerESIMDPass();
SYCLLowerESIMDPass(bool OptLevelO0);

private:
size_t runOnFunction(Function &F, SmallPtrSet<Type *, 4> &);
const bool OptLevelO0;
};

ModulePass *createSYCLLowerESIMDPass();
ModulePass *createSYCLLowerESIMDPass(bool OptLevelO0 = false);
void initializeSYCLLowerESIMDLegacyPassPass(PassRegistry &);

class ESIMDLowerLoadStorePass : public PassInfoMixin<ESIMDLowerLoadStorePass> {
Expand Down
23 changes: 19 additions & 4 deletions llvm/lib/SYCLLowerIR/LowerESIMD.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Module.h"
#include "llvm/Pass.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/raw_ostream.h"

#include <cctype>
Expand All @@ -42,14 +43,24 @@ namespace id = itanium_demangle;

#define SLM_BTI 254

// This option is only for testing. TODO: remove this option
// with the refactoring of LowerESIMD pass (ModulePass -> FunctionPass)
static cl::opt<bool> LowerEsimdOptLevelO0(
"lower-esimd-opt-level-O0", llvm::cl::Optional, llvm::cl::Hidden,
llvm::cl::desc("Force no optimizations for LowerESIMD pass"),
llvm::cl::init(false));

namespace {
SmallPtrSet<Type *, 4> collectGenXVolatileTypes(Module &);
void generateKernelMetadata(Module &);

class SYCLLowerESIMDLegacyPass : public ModulePass {
public:
static char ID; // Pass identification, replacement for typeid
SYCLLowerESIMDLegacyPass() : ModulePass(ID) {
SYCLLowerESIMDLegacyPass() : ModulePass(ID), Impl(/*OptLevelO0*/ false) {
initializeSYCLLowerESIMDLegacyPassPass(*PassRegistry::getPassRegistry());
}
SYCLLowerESIMDLegacyPass(bool OptLevelO0) : ModulePass(ID), Impl(OptLevelO0) {
initializeSYCLLowerESIMDLegacyPassPass(*PassRegistry::getPassRegistry());
}

Expand All @@ -70,10 +81,14 @@ INITIALIZE_PASS(SYCLLowerESIMDLegacyPass, "LowerESIMD",
"Lower constructs specific to Close To Metal", false, false)

// Public interface to the SYCLLowerESIMDPass.
ModulePass *llvm::createSYCLLowerESIMDPass() {
return new SYCLLowerESIMDLegacyPass();
ModulePass *llvm::createSYCLLowerESIMDPass(bool OptLevelO0) {
return new SYCLLowerESIMDLegacyPass(OptLevelO0);
}

SYCLLowerESIMDPass::SYCLLowerESIMDPass() : OptLevelO0(LowerEsimdOptLevelO0) {}
SYCLLowerESIMDPass::SYCLLowerESIMDPass(bool OptLevelO0)
: OptLevelO0(LowerEsimdOptLevelO0 ? true : OptLevelO0) {}

namespace {
// The regexp for ESIMD intrinsics:
// /^_Z(\d+)__esimd_\w+/
Expand Down Expand Up @@ -1262,7 +1277,7 @@ size_t SYCLLowerESIMDPass::runOnFunction(Function &F,
// functions to be inlined into the kernel itself. To overcome this
// limitation, mark every function called from ESIMD kernel with
// 'alwaysinline' attribute.
if ((F.getCallingConv() != CallingConv::SPIR_KERNEL) &&
if (!OptLevelO0 && (F.getCallingConv() != CallingConv::SPIR_KERNEL) &&
Comment on lines 1277 to +1280
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is not the first time I questioning overall design here: why can't we do the same transformation + launch alwaysinline pass within GPU vector backend?

It seems a bit strange to me that in target-independent tool like sycl-post-link we have quite a lot of workarounds specifically for GPU vector compute backend.

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 believe that is the limitation of the GPU backend. @kbobrovs , can you please comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that this is a limitation, I just don't understand why device-specific backend can't apply device-specific lowering to its input so it is canonicalized before performing actual work on it?

Again, I understand that technically our interface is just a SPIR-V and we haven't documented any particular optimization level which is expected by the backend, but I would expect that such things as inlining (for example) should be simple enough to be implemented in device-specific backend if it is strictly needed in there. I would be complaining less if this were a requirement for more than one backend, i.e. it would be more generic.

Clarification: I'm not trying to block this particular PR, I'm just not so happy with the approach of putting things which are unique to a particular backend into this more or less generic tool. Just trying to see if there is something we can do about that

Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexeySachkov, yes you are right. The ideal is to have ESIMD BE do all necessary preparation, including recognition of some/all of the ESIMD/SPIRV intrinsics. This has been brought up few times before. But look at this from a different perspective: we have an external BE, which is not part of the LLVM Code Gen infrastructure, and we still want to use it given the interface it provides. Luckily, the BE is not quite external, so the plan is to gradually move ESIMD-dependent parts into the BE mid/long-term, and the extra LLVM IR passes should be the first candidate.

tagging @kvladimi

Strictly speaking, these particular changes are not sycl-post-link tools changes (which serves just as an LTO driver here) and are isolated in the LowerESIMD pass. But the point you raised is valid anyway. BTW, we plan to move LowerESIMD back to BackendUtils.cpp, as having it in LTO broke few things.

!F.hasFnAttribute(Attribute::AlwaysInline))
F.addFnAttr(Attribute::AlwaysInline);

Expand Down
18 changes: 12 additions & 6 deletions llvm/test/SYCLLowerIR/esimd_lower_inline_hint.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
; RUN: opt -LowerESIMD -S < %s | FileCheck %s
; RUN: opt -LowerESIMD -S < %s | FileCheck -check-prefixes=CHECK,CHECK-ATTR %s
; RUN: opt -LowerESIMD -lower-esimd-opt-level-O0 -S < %s | FileCheck -check-prefixes=CHECK,CHECK-NO-ATTR %s

; This test checks that LowerESIMD pass sets the 'alwaysinline'
; attribute for all non-kernel functions.
Expand All @@ -20,20 +21,25 @@ define spir_kernel void @EsimdKernel2() {
}

define spir_func void @foo() {
; CHECK: @foo() #[[ATTR:[0-9]+]]
; CHECK-ATTR: @foo() #[[ATTR:[0-9]+]] {
; CHECK-NO-ATTR @foo() {
ret void
}

define spir_func void @bar() {
; CHECK: @bar() #[[ATTR]]
; CHECK-NEXT: call void @foobar
; CHECK-ATTR: @bar() #[[ATTR]] {
; CHECK-ATTR-NEXT: call void @foobar
; CHECK-NO-ATTR: @bar() {
; CHECK-NO-ATTR-NEXT: call void @foobar
call void @foobar()
ret void
}

define spir_func void @foobar() {
; CHECK: @foobar() #[[ATTR]]
; CHECK-ATTR: @foobar() #[[ATTR]] {
; CHECK-NO-ATTR: @foobar() {
ret void
}

; CHECK: attributes #[[ATTR]] = { alwaysinline }
; CHECK-ATTR: attributes #[[ATTR]] = { alwaysinline }
; CHECK-NO-ATTR-NOT: attributes {{.*}} alwaysinline
2 changes: 1 addition & 1 deletion llvm/tools/sycl-post-link/sycl-post-link.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ static string_vector saveResultSymbolsLists(string_vector &ResSymbolsLists,
// TODO: support options like -debug-pass, -print-[before|after], and others
static void LowerEsimdConstructs(Module &M) {
legacy::PassManager MPM;
MPM.add(createSYCLLowerESIMDPass());
MPM.add(createSYCLLowerESIMDPass(OptLevelO0));
if (!OptLevelO0) {
// Force-inline all functions marked 'alwaysinline' by the LowerESIMD pass.
MPM.add(createAlwaysInlinerLegacyPass());
Expand Down
4 changes: 4 additions & 0 deletions sycl/test/esimd/vadd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
// RUN: %clangxx -I %sycl_include %s -o %t.out -lsycl
// RUN: %RUN_ON_HOST %t.out

// Check that the code compiles with -O0 and -g
// RUN: %clangxx -I %sycl_include %s -o %t.out -fsycl -fsycl-explicit-simd -O0
// RUN: %clangxx -I %sycl_include %s -o %t.out -fsycl -fsycl-explicit-simd -O0 -g

#include <CL/sycl.hpp>
#include <CL/sycl/INTEL/esimd.hpp>
#include <iostream>
Expand Down