-
Notifications
You must be signed in to change notification settings - Fork 787
[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
Changes from 1 commit
5767c50
56854ed
38c1541
a0a9c53
dedf2ed
497aa64
a1aadf5
8f089a6
9d33f38
209532f
c6fed9f
992fcda
04737a2
cf0b167
9b59931
ac0422b
0ae282d
87d0f46
a073628
f1cad2f
53fd818
5a4eae3
70ae7a0
2618034
a7c301d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
#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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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<")) { | ||
kbobrovs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. So
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(), {})); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these still needed?