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.

Sign up for GitHub

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

Jump to bottom

Make BoolArrayStoreTransformer iterate CFG for DLT compiles #15393

Merged
merged 1 commit into from
Jul 11, 2022
Merged

Make BoolArrayStoreTransformer iterate CFG for DLT compiles #15393

merged 1 commit into from
Jul 11, 2022

Conversation

BradleyWood
Copy link
Member

On DLT compiles where type information is not available we must not truncate byte stores to 1-bit.

Fixes #15369

Signed-off-by: BradleyWood bradley.wood@ibm.com

@BradleyWood
Copy link
Member Author

FYI @0xdaryl

@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 23, 2022

Jenkins test sanity all jdk17

@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 24, 2022

AArch64 CI failure seems to be a variant of #13577.

@0xdaryl 0xdaryl self-assigned this Jun 24, 2022
@BradleyWood
Copy link
Member Author

x64 mac failure caused by warning:

12:03:37  /Users/jenkins/workspace/Build_JDK17_x86-64_mac_Personal/openj9/runtime/compiler/optimizer/BoolArrayStoreTransformer.cpp:155:40: error: '&&' within '||' [-Werror,-Wlogical-op-parentheses]
12:03:37        if (_hasByteArrayAutoOrCheckCast && _hasBoolArrayAutoOrCheckCast || comp()->isDLT())

I'll add parenthesis...

@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 24, 2022

Jenkins test sanity all jdk17

1 similar comment
@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 27, 2022

Jenkins test sanity all jdk17

0xdaryl
0xdaryl reviewed Jun 28, 2022
View reviewed changes
Copy link
Contributor

@0xdaryl 0xdaryl left a comment

Choose a reason for hiding this comment

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

Seems reasonable, but @jdmpapin would you mind offering an opinion on the fix since I believe you reviewed the initial implementation.

jdmpapin
jdmpapin approved these changes Jul 6, 2022
View reviewed changes
Copy link
Contributor

@jdmpapin jdmpapin left a comment

Choose a reason for hiding this comment

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

Approved assuming that the comment/commit message will be clarified

A similar problem is possible when parameters are overwritten by bytecode instructions that don't get translated to trees for DLT, and this transformation can currently trust the parameter signature when it shouldn't. However, in determining that this problem exists, I've fixed it myself. This PR is already a strict improvement, so I think it should be merged as-is, and I will open a follow-up fix afterward

runtime/compiler/optimizer/BoolArrayStoreTransformer.cpp Outdated Show resolved Hide resolved
@BradleyWood
BoolArrayStoreTransformer: check CFG on DLT comp
1abf31f 
On DLT compiles full analysis is needed because
not all bytecodes are translated into IL. Autos
and/or checkcast operations of type boolean[]
and byte[] may exist.

Fixes eclipse-openj9#15369

Signed-off-by: BradleyWood <bradley.wood@ibm.com>
@jdmpapin
Copy link
Contributor

jdmpapin commented Jul 7, 2022 •
edited

@0xdaryl, are you ok with merging this now? (I imagine so, but just want to confirm)

@0xdaryl 0xdaryl merged commit e102f30 into eclipse-openj9:master Jul 11, 2022
@jdmpapin jdmpapin mentioned this pull request Jul 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xdaryl 0xdaryl 0xdaryl approved these changes

@jdmpapin jdmpapin jdmpapin approved these changes

Assignees

@0xdaryl 0xdaryl

Labels
comp:jit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compilation Fails even from noOpt Level
3 participants
@BradleyWood @0xdaryl @jdmpapin