-
Notifications
You must be signed in to change notification settings - Fork 738
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
[SYCL][Fusion][NoSTL] Implement custom view class to replace input vector references #12401
Conversation
afe4fd3
to
39be3ab
Compare
ed04164
to
fd0e48e
Compare
fd0e48e
to
69aecc5
Compare
69aecc5
to
f04716c
Compare
sycl/source/detail/jit_compiler.cpp
Outdated
@@ -630,6 +630,10 @@ updatePromotedArgs(const ::jit_compiler::SYCLKernelInfo &FusedKernelInfo, | |||
} | |||
} | |||
|
|||
template <typename T> static auto getView(const std::vector<T> &Vec) { |
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.
Would it make sense to collect those helpers in one place (header)?
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.
You mean getView
, toArrayRef
and toVector
, right? I think toArrayRef
can't live together with the others because of the reliance of the LLVM headers. (NB: If I'm wrong and we can include those, we could just use ArrayRef
directly.)
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.
From what I can tell, the helpers are only used in .cpp
files. Could we not place them in a new header in common
that is not exposed to the outside, but allows us to define the helpers in a single place and use across different source files within the JIT compiler?
As this header would be internal to the JIT compiler only, including LLVM components would not be problematic.
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.
I channeled my inner @victor-eds and found a template-based solution: View
(dropped the Vector
, should be specific enough as it is in the jit_compiler
namespace) can now be constructed from anything that offers data()
and size()
, and there is a templated to
method which can return any type that can be constructed from begin()
and end()
iterators.
f04716c
to
a2723de
Compare
…ctor references Signed-off-by: Julian Oppermann <julian.oppermann@codeplay.com>
a2723de
to
707acbe
Compare
Implement a very simple view class to transport the input vector's raw data pointers and sizes across the fusion interface, and modify
FusionResult
to work withoutstd::string
andstd::variant
.This PR is part of a series of changes to remove uses of STL classes in the kernel fusion interface to prevent ABI issues in the future.