Skip to content

[SYCL][ESIMD]Limit propagation of ESIMD attributes #7191

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 25 commits into from
Dec 5, 2022
Merged
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
5767c50
Limit propagation of ESIMD attributes to RoundedRangeKernel
fineg74 Oct 26, 2022
56854ed
Merge remote-tracking branch 'origin/sycl' into esimdSplitFix
fineg74 Nov 1, 2022
38c1541
Refactorcode to reduce code duplication
fineg74 Nov 2, 2022
a0a9c53
Fix compilation issue on RHEL
fineg74 Nov 2, 2022
dedf2ed
Move allocator to a utils file
fineg74 Nov 3, 2022
497aa64
Merge remote-tracking branch 'origin/sycl' into esimdSplitFix
fineg74 Nov 9, 2022
a1aadf5
Prevent traverseCallgraphUp from traversing beyond invoke_simd call
fineg74 Nov 10, 2022
8f089a6
Address PR comments
fineg74 Nov 10, 2022
9d33f38
Address PR comments
fineg74 Nov 10, 2022
209532f
Address PR comments
fineg74 Nov 22, 2022
c6fed9f
Address PR comments
fineg74 Nov 23, 2022
992fcda
Address PR comments
fineg74 Nov 23, 2022
04737a2
Address PR comments
fineg74 Nov 29, 2022
cf0b167
Address PR comments
fineg74 Nov 29, 2022
9b59931
Address PR comments
fineg74 Nov 29, 2022
ac0422b
Update llvm/include/llvm/SYCLLowerIR/ESIMD/ESIMDUtils.h
fineg74 Nov 29, 2022
0ae282d
Fix clang-format issue
fineg74 Nov 30, 2022
87d0f46
Update llvm/include/llvm/SYCLLowerIR/SYCLUtils.h
fineg74 Nov 30, 2022
a073628
Update llvm/lib/SYCLLowerIR/ESIMD/LowerESIMDKernelAttrs.cpp
fineg74 Nov 30, 2022
f1cad2f
Update llvm/lib/SYCLLowerIR/ESIMD/LowerESIMDKernelAttrs.cpp
fineg74 Nov 30, 2022
53fd818
Fix clang-format issue.
fineg74 Nov 30, 2022
5a4eae3
Update llvm/include/llvm/SYCLLowerIR/SYCLUtils.h
fineg74 Nov 30, 2022
70ae7a0
Fix handling of StoreInst
fineg74 Nov 30, 2022
2618034
Merge branch 'esimdSplitFix' of https://github.com/fineg74/llvm into …
fineg74 Nov 30, 2022
a7c301d
Address PR comments
fineg74 Dec 1, 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
62 changes: 58 additions & 4 deletions llvm/lib/SYCLLowerIR/ESIMD/LowerESIMDKernelAttrs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,50 @@
// Finds and adds sycl_explicit_simd attributes to wrapper functions that wrap
// ESIMD kernel functions

#include "llvm/Demangle/Demangle.h"
#include "llvm/Demangle/ItaniumDemangle.h"
#include "llvm/IR/Module.h"
#include "llvm/Pass.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

are these still needed?

#include "llvm/SYCLLowerIR/ESIMD/ESIMDUtils.h"
#include "llvm/SYCLLowerIR/ESIMD/LowerESIMD.h"
#include "llvm/SYCLLowerIR/SYCLUtils.h"

#include "llvm/IR/Module.h"
#include "llvm/Pass.h"

#define DEBUG_TYPE "LowerESIMDKernelAttrs"

using namespace llvm;

namespace {
Copy link
Contributor

Choose a reason for hiding this comment

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

This allocator is used in other places - please factor this out to a separate source to avoid code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// Simplest possible implementation of an allocator for the Itanium demangler
class SimpleAllocator {
protected:
SmallVector<void *, 128> Ptrs;

public:
void reset() {
for (void *Ptr : Ptrs) {
// Destructors are not called, but that is OK for the
// itanium_demangle::Node subclasses
std::free(Ptr);
}
Ptrs.resize(0);
}

template <typename T, typename... Args> T *makeNode(Args &&...args) {
void *Ptr = std::calloc(1, sizeof(T));
Ptrs.push_back(Ptr);
return new (Ptr) T(std::forward<Args>(args)...);
}

void *allocateNodeArray(size_t sz) {
void *Ptr = std::calloc(sz, sizeof(llvm::itanium_demangle::Node *));
Ptrs.push_back(Ptr);
return Ptr;
}

~SimpleAllocator() { reset(); }
};
} // namespace

namespace llvm {
PreservedAnalyses
SYCLFixupESIMDKernelWrapperMDPass::run(Module &M, ModuleAnalysisManager &MAM) {
Expand All @@ -30,7 +63,28 @@ SYCLFixupESIMDKernelWrapperMDPass::run(Module &M, ModuleAnalysisManager &MAM) {
sycl::utils::traverseCallgraphUp(
&F,
[&](Function *GraphNode) {
if (!llvm::esimd::isESIMD(*GraphNode)) {
if (!llvm::esimd::isESIMD(*GraphNode) &&
llvm::esimd::isKernel(*GraphNode)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This filtering leaves out SYCL_EXTERNAL functions, which does not seem correct.


// Demangle the caller name to check if the function is called
// from RoundedRangeKernel.
StringRef MangledName = GraphNode->getName();
llvm::itanium_demangle::ManglingParser<SimpleAllocator> Parser(
MangledName.begin(), MangledName.end());
itanium_demangle::Node *AST = Parser.parse();
if (!AST ||
AST->getKind() != itanium_demangle::Node::KSpecialName) {
return;
}

itanium_demangle::OutputBuffer NameBuf;
AST->print(NameBuf);
StringRef Name(NameBuf.getBuffer(), NameBuf.getCurrentPosition());

if (!Name.contains("sycl::_V1::detail::RoundedRangeKernel<")) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the GraphNode->setMetadata... call below is reached only when GraphNode is a RoundedRangeKernel, which means only RoundedRangeKernel gets the marker, all others are left out. Previous logic was that all functions were marked. This needs additional investigation. I'll also give this a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally this pass was introduced to resolve a problem of mixing ESIMD and NonESIMD kernels and back then the issue was determined as
The immediate cause of the issue is that ESIMD kernel is wrapped with RoundedRangeKernel and sycl_explicit_esimd attribute is not propagated to the wrapper class. As a result ModuleSplitter in sycl-post-link creates a spurious split for the wrapper and later creates a spurious non-esimd module and pulls called functions into it. As a result non-esimd module would contain esimd functions and later fails to link. It looks like that for named ESIMD kernels the attributes are properly propagated and therefore the issue doesn't occur. (#6557)
This bug basically says that marking all functions that call ESIMD functions is also incorrect as it results in a crash in some circumstances. Therefore the fix here is to limit marking to RoundedRangeKernel which was the original source of the problem. So in this sense the fix does what it is expected to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

What this patch does is it excludes all the functions in the caller chain from given ESIMD function F up to RoundedRangeKernel from marking them as ESIMD, instead, it only marks the kernel. Previously all the function in the call chain were marked.
Now, this patch is supposed to fix internal LLVM error on some function met in the IR:
Assertion `isa(Val) && "cast() argument of incompatible type!"' failed.

So

  1. All the functions in a call chain ending at ESIMD function must be ESIMD functions, so not marking them is incorrect.
  2. I think the patch just masks the problem by skipping marking the problematic function (which in turn can spawn other problems).

I think we need to come up with a patch that fixes the root cause of the above assert.

GraphNode->setMetadata(
llvm::esimd::ESIMD_MARKER_MD,
llvm::MDNode::get(GraphNode->getContext(), {}));
Expand Down