-
Notifications
You must be signed in to change notification settings - Fork 397
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
Transform new converter call to arraytranslate node #7247
Conversation
Tagging @r30shah @hzongaro @vijaysun-omr for review. |
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 think the changes look good, though I will defer to @r30shah for a more thorough review.
May I ask you to add a little more detail to the commit message? Other than that suggestion, I have just a couple of cosmetic comments.
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.
Some minor nitpicks, but overall it looks good to me.
@zl-wang and @knn-k / @Akira1Saitoh fyi |
AArch64 codegen does not implement arraytranslate now. Issue #6092. |
3d75239
to
08ce7c0
Compare
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.
Changes looks good to me. Just a minor question. No changes needed in the code.
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.
Looks good. Thanks!
08ce7c0
to
81f30e4
Compare
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.
LGTM
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 had a question about the use of the environment variable to control this transformation.
StringCoding.implEncodeAsciiArray is a renamed version of US_ASCII/Encoder.encodeASCII, which is a recognized method. This commit introduces infrastructure to enable inlining of implEncodeAsciiArray like it is done for encodeASCII.
81f30e4
to
7c2e3e1
Compare
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.
Looks good. Thanks!
Jenkins build all |
x86-64 macOS test failure is due to known issue #7181 |
OMR testing was successful, other than failures due to known issues. Downstream OpenJ9 testing with this change was also successful. Merging. |
StringCoding.implEncodeAsciiArray
is nearly identical in implementation and use case to the oldUS_ASCII/Encoder.encodeASCII
method. This PR adds support in VP for transformingStringCoding.implEncodeAsciiArray
into anarraytranslate
that can then be take advantage of pre-existing acceleration for this workload.