Skip to content

[SYCL][NFC] Optimize getKernelNamesUsingAssert #5196

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
bader merged 5 commits into from
Dec 27, 2021
Merged

[SYCL][NFC] Optimize getKernelNamesUsingAssert #5196

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
747e14d
[SYCL] optimize getKernelNamesUsingAssert
maksimsab Dec 21, 2021
7d4b973
fix review remarks
maksimsab Dec 22, 2021
d734118
Adjust with LLVM coding style
maksimsab Dec 22, 2021
0a0df33
sort kernel's names which use asserts. refuse of CHECK-DAG.
maksimsab Dec 24, 2021
8350b9c
remove sorting of kernel's names
maksimsab Dec 27, 2021
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
14 changes: 7 additions & 7 deletions llvm/test/tools/sycl-post-link/assert-indirect-with-split-2.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,6 @@ entry:
ret void
}

; CHECK-NOT: main_TU0_kernel1
define dso_local spir_kernel void @main_TU0_kernel1() #0 {
entry:
call spir_func void @_Z4foo1v()
ret void
}

; Function Attrs: nounwind
define dso_local spir_func void @_Z4foo1v() {
entry:
Expand All @@ -85,6 +78,13 @@ entry:
ret void
}

; CHECK-NOT: main_TU0_kernel1
define dso_local spir_kernel void @main_TU0_kernel1() #0 {
entry:
call spir_func void @_Z4foo1v()
ret void
}

Comment thread
AlexeySachkov marked this conversation as resolved.
Outdated

; This function is marked with "referenced-indirectly", but it doesn't call an assert
; Function Attrs: nounwind
Expand Down
36 changes: 18 additions & 18 deletions llvm/test/tools/sycl-post-link/assert-property-2.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,15 @@ entry:
ret void
}

; CHECK: _ZTSZZ4mainENKUlRN2cl4sycl7handlerEE_clES2_E7Kernel9
; CHECK-DAG: _ZTSZZ4mainENKUlRN2cl4sycl7handlerEE_clES2_E7Kernel9
; Function Attrs: convergent noinline norecurse mustprogress
define weak_odr dso_local spir_kernel void @_ZTSZZ4mainENKUlRN2cl4sycl7handlerEE_clES2_E7Kernel9() #0 {
entry:
call spir_func void @_Z1Jv()
ret void
}

; CHECK: _ZTSZZ4mainENKUlRN2cl4sycl7handlerEE_clES2_E8Kernel10
; CHECK-DAG: _ZTSZZ4mainENKUlRN2cl4sycl7handlerEE_clES2_E8Kernel10
; Function Attrs: convergent noinline norecurse optnone mustprogress
define weak_odr dso_local spir_kernel void @_ZTSZZ4mainENKUlRN2cl4sycl7handlerEE_clES2_E8Kernel10() #0 {
entry:
Expand Down Expand Up @@ -164,7 +164,7 @@ entry:
ret void
}

; CHECK: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE6Kernel
; CHECK-DAG: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE6Kernel
; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE6Kernel"() local_unnamed_addr #0 {
entry:
Expand All @@ -186,7 +186,7 @@ entry:
ret void
}

; CHECK: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel2
; CHECK-DAG: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel2
; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel2"() local_unnamed_addr #0 {
entry:
Expand Down Expand Up @@ -216,7 +216,7 @@ entry:
ret void
}

; CHECK: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel3
; CHECK-DAG: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel3
; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel3"() local_unnamed_addr #0 {
entry:
Expand Down Expand Up @@ -244,15 +244,15 @@ entry:
ret void
}

; CHECK: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel4
; CHECK-DAG: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel4
; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel4"() local_unnamed_addr #0 {
entry:
call spir_func void @_Z7common2v()
ret void
}

; CHECK: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel5
; CHECK-DAG: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel5
; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel5"() local_unnamed_addr #0 {
entry:
Expand All @@ -267,23 +267,14 @@ entry:
ret void
}

; CHECK-NOT: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel
; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel6"() local_unnamed_addr #0 {
entry:
call spir_func void @_Z6E_exclv()
call spir_func void @_Z6E_exclv()
ret void
}

; Function Attrs: convergent norecurse nounwind mustprogress
define dso_local spir_func void @_Z6F_inclv() local_unnamed_addr {
entry:
call spir_func void @_Z11assert_funcv()
ret void
}

; CHECK: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel7
; CHECK-DAG: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel7
; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel7"() local_unnamed_addr #0 {
entry:
Expand Down Expand Up @@ -328,14 +319,23 @@ entry:
ret void
}

; CHECK: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel8
; CHECK-DAG: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel8
; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel8"() local_unnamed_addr #0 {
call spir_func void @_Z1Gv()
call spir_func void @_Z1Hv()
ret void
}

; CHECK-NOT: _ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel6

@AlexeySachkov AlexeySachkov Dec 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please note that CHECK-NOT is bounded by surrounding CHECK directives. If you want to ensure that some patters is not present in the file at all, then you should use --implicit-check-not

@maksimsab maksimsab Dec 24, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, today I learnt that implicit-check-not option doesn't work with CHECK-DAG. At least, in a way which I got from documentation.
For example, consider the following inputs:
file:

a
b
c

ll:

; CHECK-DAG: a
; CHECK-DAG: c

If you run FileCheck ll -input-file=./file --implicit-check-not b then you get exit code 0 while I would expect to get non-zero.

@AlexeySachkov AlexeySachkov Dec 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unfortunately, today I learnt that implicit-check-not option doesn't work with CHECK-DAG. At least, in a way which I got from documentation.

Yeah, I see it too now.

I did that kernel' names print in a sorted order so that it would be easier to check them with simple CHECK:. You might have an approach better than mine.

We shouldn't be doing extra actions like sorting strings just to make our unit-tests pass. Better solution would be to still work on tests: let's just have two separate FileCheck runs on the same input, but with different prefixes (--check-prefix). The first one will check that expected names are present in the result through CHECK-DAG, the second one will only check that unexpected names are not present in the result.

@maksimsab maksimsab Dec 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Done that way.

; Function Attrs: convergent norecurse mustprogress
define weak_odr dso_local spir_kernel void @"_ZTSZZ4mainENK3$_0clERN2cl4sycl7handlerEE7Kernel6"() local_unnamed_addr #0 {
entry:
call spir_func void @_Z6E_exclv()
call spir_func void @_Z6E_exclv()
ret void
}

; Function Attrs: convergent norecurse mustprogress
define weak dso_local spir_func void @__assert_fail(i8 addrspace(4)* %expr, i8 addrspace(4)* %file, i32 %line, i8 addrspace(4)* %func) local_unnamed_addr {
entry:
Expand Down
16 changes: 8 additions & 8 deletions llvm/test/tools/sycl-post-link/assert-property-with-split.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ target triple = "spir64-unknown-linux"

; CHECK: [SYCL/assert used]

; CHECK: _ZTSZ4mainE11TU0_kernel0
; CHECK-DAG: _ZTSZ4mainE11TU0_kernel0
define dso_local spir_kernel void @_ZTSZ4mainE11TU0_kernel0() #0 {
entry:
call spir_func void @_Z3foov()
Expand All @@ -36,6 +36,13 @@ entry:
ret void
}

; CHECK-DAG: _ZTSZ4mainE10TU1_kernel
define dso_local spir_kernel void @_ZTSZ4mainE10TU1_kernel() #1 {
entry:
call spir_func void @_Z4foo2v()
ret void
}

; CHECK-NOT: _ZTSZ4mainE11TU0_kernel1
define dso_local spir_kernel void @_ZTSZ4mainE11TU0_kernel1() #0 {
entry:
Expand All @@ -51,13 +58,6 @@ entry:
ret void
}

; CHECK: _ZTSZ4mainE10TU1_kernel
define dso_local spir_kernel void @_ZTSZ4mainE10TU1_kernel() #1 {
entry:
call spir_func void @_Z4foo2v()
ret void
}

; Function Attrs: nounwind
define dso_local spir_func void @_Z4foo2v() {
entry:
Expand Down
166 changes: 63 additions & 103 deletions 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 @@ -46,7 +46,10 @@
#include <algorithm>
#include <map>
#include <memory>
#include <queue>
#include <string>
#include <unordered_set>
#include <utility>
#include <vector>

using namespace llvm;
Expand Down Expand Up @@ -352,124 +355,81 @@ void groupEntryPoints(const Module &M, EntryPointGroupMap &EntryPointsGroups,
EntryPointsGroups[GLOBAL_SCOPE_NAME] = {};
}

enum HasAssertStatus { No_Assert, Assert, Assert_Indirect };

// Go through function call graph searching for assert call.
HasAssertStatus hasAssertInFunctionCallGraph(const Function *Func) {
// Map holds the info about assertions in already examined functions:
// true - if there is an assertion in underlying functions,
// false - if there are definetely no assertions in underlying functions.
static std::map<const Function *, bool> hasAssertionInCallGraphMap;
std::vector<const Function *> FuncCallStack;

static std::vector<const Function *> isIndirectlyCalledInGraph;

std::vector<const Function *> Workstack;
Workstack.push_back(Func);

while (!Workstack.empty()) {
const Function *F = Workstack.back();
Workstack.pop_back();
if (F != Func)
FuncCallStack.push_back(F);

bool HasIndirectlyCalledAttr = false;
if (std::find(isIndirectlyCalledInGraph.begin(),
isIndirectlyCalledInGraph.end(),
F) != isIndirectlyCalledInGraph.end())
HasIndirectlyCalledAttr = true;
else if (F->hasFnAttribute("referenced-indirectly")) {
HasIndirectlyCalledAttr = true;
isIndirectlyCalledInGraph.push_back(F);
}
// This function traverses over reversed call graph by BFS algorithm.
// It means that an edge links some function @func with functions
// which contain call of function @func.It starts from
Comment thread
maksimsab marked this conversation as resolved.
Outdated
// @StartingFunction and lifts up until it reach all reachable functions
// or it reaches some function containing "referenced-indirectly" attribute.
// If it reaches "referenced-indirectly" attribute than it returns true and
// an empty list.
// Otherwise, it returns false and a list of reached SPIR kernel function's
// names.
std::pair<bool, std::vector<StringRef>>
Comment thread
maksimsab marked this conversation as resolved.
Outdated
TraverseCGToFindSPIRKernels(const Function *StartingFunction) {
std::queue<const Function *> FunctionsToVisit;
std::unordered_set<const Function *> VisitedFunctions;
FunctionsToVisit.push(StartingFunction);
std::vector<StringRef> KernelNames;

while (!FunctionsToVisit.empty()) {
const Function *F = FunctionsToVisit.front();
FunctionsToVisit.pop();

// It is possible that we insert some particular function several
// times in functionsToVisit queue.
if (VisitedFunctions.find(F) != VisitedFunctions.end())
continue;
Comment thread
maksimsab marked this conversation as resolved.
Outdated

bool IsLeaf = true;
for (const auto &I : instructions(F)) {
if (!isa<CallBase>(&I))
continue;
VisitedFunctions.insert(F);

const Function *CF = cast<CallBase>(&I)->getCalledFunction();
if (!CF)
for (const auto *U : F->users()) {
const Instruction *I = cast<const Instruction>(U);
const Function *ParentF = I->getFunction();

@AlexeySachkov AlexeySachkov Dec 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm concerned that there could be other uses of function, which are not instructions, but ConstantExpr. To generate such LLVM IR we can try to cast one function pointer type into another before doing a call. There is an example of such LLVM IR in the translator repo

Another concern is that theoretically, an Instruction can be located in module scope, i.e. outside of a function. Therefore, I->getFunction() will return nullptr. There is an example of such LLVM IR in the translator repo as well. Note: I don't have good idea about how to generate such LLVM IR out of a SYCL app if you want to replicate it yourself.

@maksimsab maksimsab Dec 24, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added dyn_cast to CallInst.

@AlexeySachkov AlexeySachkov Dec 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please note that even though you have fixed a nullptr dereference, functionally we still have an issue, i.e. we can say that there are no assert uses whilst there are through more complex usages of function pointers.

That is probably a minor problem at the moment, because function pointers are not widely used, but we should leave a FIXME here, at least, mentioning the use cases we don't analyze properly.

@maksimsab maksimsab Dec 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Currently, there is a kernel empty_kernel in assert-indirect-with-split-2 which checks that traversing doesn't go in such cases.

if (VisitedFunctions.find(ParentF) != VisitedFunctions.end())
Comment thread
maksimsab marked this conversation as resolved.
Outdated
continue;

bool IsIndirectlyCalled =
HasIndirectlyCalledAttr ||
std::find(isIndirectlyCalledInGraph.begin(),
isIndirectlyCalledInGraph.end(),
CF) != isIndirectlyCalledInGraph.end();

// Return if we've already discovered if there are asserts in the
// function call graph.
auto HasAssert = hasAssertionInCallGraphMap.find(CF);
if (HasAssert != hasAssertionInCallGraphMap.end()) {
// If we know, that this function does not contain assert, we still
// should investigate another instructions in the function.
if (!HasAssert->second)
continue;

return IsIndirectlyCalled ? Assert_Indirect : Assert;
if (ParentF->hasFnAttribute("referenced-indirectly")) {
return {true, {}};
}

if (CF->getName().startswith("__devicelib_assert_fail")) {
// Mark all the functions above in call graph as ones that can call
// assert.
for (const auto *It : FuncCallStack)
hasAssertionInCallGraphMap[It] = true;

hasAssertionInCallGraphMap[Func] = true;
hasAssertionInCallGraphMap[CF] = true;

return IsIndirectlyCalled ? Assert_Indirect : Assert;
}
if (ParentF->getCallingConv() == CallingConv::SPIR_KERNEL)
KernelNames.push_back(ParentF->getName());

if (!CF->isDeclaration()) {
Workstack.push_back(CF);
IsLeaf = false;
if (HasIndirectlyCalledAttr)
isIndirectlyCalledInGraph.push_back(CF);
}
}

if (IsLeaf && !FuncCallStack.empty()) {
// Mark the leaf function as one that definetely does not call assert.
hasAssertionInCallGraphMap[FuncCallStack.back()] = false;
FuncCallStack.clear();
FunctionsToVisit.push(ParentF);
}
}
return No_Assert;

return {false, std::move(KernelNames)};
}

std::vector<StringRef> getKernelNamesUsingAssert(const Module &M) {
std::vector<StringRef> Result;

bool HasIndirectlyCalledAssert = false;
EntryPointGroup Kernels;
for (const auto &F : M.functions()) {
// TODO: handle SYCL_EXTERNAL functions for dynamic linkage.
// TODO: handle function pointers.
if (F.getCallingConv() != CallingConv::SPIR_KERNEL)
continue;

Kernels.push_back(&F);
if (HasIndirectlyCalledAssert)
continue;

HasAssertStatus HasAssert = hasAssertInFunctionCallGraph(&F);
switch (HasAssert) {
case Assert:
Result.push_back(F.getName());
break;
case Assert_Indirect:
HasIndirectlyCalledAssert = true;
break;
case No_Assert:
break;
Optional<const Function *> DevicelibAssertFailFunction;
std::vector<StringRef> SPIRKernelNames;
// This loop finds all SPIR kernel's names and __devicelib_assert_fail
// function if it is present.
Comment thread
maksimsab marked this conversation as resolved.
Outdated
for (const Function &F : M) {
if (F.getCallingConv() == CallingConv::SPIR_KERNEL)
SPIRKernelNames.push_back(F.getName());

if (F.getName().startswith("__devicelib_assert_fail")) {
assert(!DevicelibAssertFailFunction.hasValue());
DevicelibAssertFailFunction = &F;
}
}

if (HasIndirectlyCalledAssert)
for (const auto *F : Kernels)
Result.push_back(F->getName());
if (!DevicelibAssertFailFunction)
return {};

auto TraverseResult =
TraverseCGToFindSPIRKernels(*DevicelibAssertFailFunction);
std::vector<StringRef> Result;
if (TraverseResult.first) {
// If assert is met in some indirectly callable function than
// we return all kernels in Module due to the current assert's design.
Result = std::move(SPIRKernelNames);
} else {
Result = std::move(TraverseResult.second);
}

return Result;
}
Expand Down