Skip to content
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

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

dchopra001
Copy link
Contributor

StringCoding.implEncodeAsciiArray is nearly identical in implementation and use case to the old US_ASCII/Encoder.encodeASCII method. This PR adds support in VP for transforming StringCoding.implEncodeAsciiArray into an arraytranslate that can then be take advantage of pre-existing acceleration for this workload.

@dchopra001
Copy link
Contributor Author

Tagging @r30shah @hzongaro @vijaysun-omr for review.

@hzongaro hzongaro self-assigned this Feb 6, 2024
Copy link
Contributor

@hzongaro hzongaro left a 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.

compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@r30shah r30shah left a 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.

compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Show resolved Hide resolved
compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
@vijaysun-omr
Copy link
Contributor

@zl-wang and @knn-k / @Akira1Saitoh fyi

@knn-k
Copy link
Contributor

knn-k commented Feb 8, 2024

AArch64 codegen does not implement arraytranslate now. Issue #6092.

@dchopra001 dchopra001 force-pushed the accelImplEncodeAscii branch 2 times, most recently from 3d75239 to 08ce7c0 Compare February 12, 2024 17:21
Copy link
Contributor

@r30shah r30shah left a 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.

compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hzongaro hzongaro left a 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.

compiler/optimizer/ValuePropagationCommon.cpp Outdated Show resolved Hide resolved
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.
Copy link
Contributor

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@hzongaro
Copy link
Contributor

Jenkins build all

@hzongaro
Copy link
Contributor

x86-64 macOS test failure is due to known issue #7181

@hzongaro
Copy link
Contributor

OMR testing was successful, other than failures due to known issues. Downstream OpenJ9 testing with this change was also successful. Merging.

@hzongaro hzongaro merged commit 3d47c69 into eclipse-omr:master Feb 16, 2024
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants