Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Initial changes for C++11 ABI=0 support #12193
[SYCL] Initial changes for C++11 ABI=0 support #12193
Changes from 112 commits
2d34c19
4dabfd3
108bf4d
d7f21ab
34cd0b9
ded92db
77aaeba
fa6a931
5709e28
da8ab9f
841f042
a79d58f
74bb97d
7cebd8d
20448f7
95f48f8
f0aa89c
a1e4479
312cda8
1db3ca8
d83490d
273f4b0
ac1ee03
436288b
84f8e2d
db67a87
66c0405
4823b25
cff7fae
3af523e
f918818
316627a
741db80
2db4e56
22a59c6
4e28f9f
e24da9c
f707404
272e400
67ecf71
e385649
8a9aaaf
2adf0fb
a309a8d
537491d
8fc92b8
f9a9af4
0aa4158
b51cb8a
6e7da0a
05a0ca1
ecb8ce9
6f36028
f4655aa
a04fc0c
a803e6d
a22fd4a
429c7a3
b18eb02
4f0d634
9d9cf42
921b754
bf313f1
32a0401
431ac8d
a4c2ffa
273248b
182afc7
0a97845
d62e175
3fdd985
8d1fa21
232e759
019d30b
36e5dd8
b074631
524d53d
4f9b8d4
fa1ae30
7095cf7
d6c7ddc
82febee
1748dea
5a37c73
7780ca5
3bc950d
0bf1237
f31b89c
037a13b
962fa26
d6ab3ca
7eb7314
a11280e
f85a76a
1327442
05dc42d
050509e
dd8e563
ffa36e1
5c0b8a1
28ad4fc
823614d
c702858
f1bf10d
43a7823
37be2ba
82908d5
1ca910c
d88422a
b48f868
6e18644
0d19467
85b7d36
b79a347
1650d6d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Every use of this seems to expect an
std::string
. Do we really need to change this data member, or maybe just a few places where it's getting assigned to?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.
Yes, this private data member should cross the ABI boundary.
So I had to convert it to detail::string to overcome the ABI incompatibility issue.
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.
Oh, that brings another question. How are you testing that there are no
std::string
s crossing the boundary?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.
Also, have you tried changing callees accepting
MKernelName
instead of modifying the callers?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 can't check everywhere at this moment.
That's why this PR is only the initial changes as the title of PR says.
We will fix as we encounter from the customer's usage case.
I am not sure if I understand this question correctly, so please correct me if I misunderstood you.
MKernelName is defined in the headerfile, and the callees (cpp files) need this data member.
So, they accept MKernelName, but I can't hand over std::string since it is crossing the ABI boundary.
So, I changed MKernelName to detail::string and then the callee re-construct std::string in the CPP file.
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.
Do they really need
std::string
? Can they keep usingdetail::string
ordetail::string_view
?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.
Well.. That's called code pollution.
We (including @sergey-semenov) do not want to propagate
detail::string
deep on the CPP side.detail::string
anddetail::string_view
should be limited to be used when crossing the ABI boundary. They are not meant to replacestd::string
andstd::string_view
.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 only partially agree with that. I think many of these interfaces currently accept
const std::string &
and they have no reason to ask for that and not forstd::string_view
. And if so, then everything can work automatically without us making ugly#ifdef
s: https://godbolt.org/z/Wc8Kd14Y8.