Enable boolean store simplification and disable other CFG simplifications #8005
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Marking this pull request as a draft as it exposes the bug reported in downstream project OpenJ9 issue eclipse-openj9/openj9#22835. |
hzongaro
force-pushed
the
enable-boolean-store-disable-other-cfg-simplifications
branch
2 times, most recently
from
6637ba7 to
b0ce21a
Compare
hzongaro
force-pushed
the
enable-boolean-store-disable-other-cfg-simplifications
branch
from
b0ce21a to
d6b795c
Compare
Contributor
Author
|
Rebased to resolve merge conflicts. |
hzongaro
force-pushed
the
enable-boolean-store-disable-other-cfg-simplifications
branch
from
d6b795c to
baeadad
Compare
Contributor
Author
|
@vijaysun-omr, may I ask you to review this change? |
| if (disableSimplifyExplicitNULLTest != NULL || disableSimplifyNullToException != NULL) | ||
| static const char *enableSimplifyExplicitNULLTest = feGetEnv("TR_enableSimplifyExplicitNULLTest"); | ||
| static const char *enableSimplifyNullToException = feGetEnv("TR_enableSimplifyNullToException"); | ||
| if (enableSimplifyExplicitNULLTest == NULL || enableSimplifyNullToException == NULL) |
Contributor
There was a problem hiding this comment.
should this condition be using && instead of || ? In that if either enable is set then we do not return false early ?
Contributor
Author
There was a problem hiding this comment.
I had debated about whether to use && or ||. If you look at the downstream pull request eclipse-openj9/openj9#22838, I did something similar there. I agree that using && would make things easier for developers to investigate these transformations in the future. I will correct this in both pull requests.
Several patterns and corresponding transformations in CFG Simplification have never been fully tested - in particular, simplifyNullToException, simplifySimpleStore, simplifyCondStoreSequence, simplifyInstanceOfTestToCheckcast and simplifyBoundCheckWithThrowException. This change disables those transformations by default, allowing them to be enabled based on the settings of various environment variables. The intention is that they should be more thoroughly tested and then reenabled as that testing and any necessary bug fixes have been completed. This change also fixes some typos in trace messages and changes the declarations of some pointers that hold the results of calls to feGetEnv from 'static char *' to 'static const char *' to emphasize that the strings pointed to will not change. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
The simplifyBooleanStore method of CFG Simplifier was disabled several years ago. This change reenables that transformation which had previously been in use for many years. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
The simplifyBooleanStore method of CFG Simplifier tries to determine whether two stores in different blocks update the same location based on a boolean condition. One of the conditions considers whether either store is storing an internal pointer. However, the code only tested whether the first of the two stores was indirect before deciding whether the first child or second child of both stores needed to have its isInternalPointer flag tested. If the first store was indirect, but the second was direct, that would result in a non-existent second child of the second store being accessed. Similarly, if the first store was direct, but the second was indirect, the first child of the second store would be accessed, rather than the child representing the value to be stored. This change fixes this problem by moving the code that performs these tests after a test of whether the two stores use the same opcode value. At that point, the stores must both be direct stores or both be indirect stores, so the same child index of each will need to have its isInternalPointer flag tested. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
hzongaro
force-pushed
the
enable-boolean-store-disable-other-cfg-simplifications
branch
from
baeadad to
7041a26
Compare
Contributor
|
Jenkins build all |
vijaysun-omr
approved these changes
Jan 20, 2026
Contributor
Author
|
Contributor
|
Merging since issues are known ones... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request is comprised of three changes related to the CFG Simplification optimization:
It re-enables the boolean store simplification of CFG Simplifier.
It fixes a bug in boolean store simplification in examining the values used in two stores on both sides of an if-then-else. The code was testing whether either value that was being stored was an internal pointer. However, it got the values of both stores based on whether the first of the two stores was indirect: if it was indirect, the value of the store must be the child at index 1; otherwise, the value of the store must be the child at index 0. However, if the first store was indirect, but the second was direct, it would access a non-existent child of the second store at index 1. Fixed this by delaying retrieving the values until after comparing the opcodes of the two stores for equality, ensuring both are indirect stores or both are direct stores.
Several patterns and corresponding transformations in CFG Simplification have never been fully tested — in particular,
simplifyNullToException,simplifySimpleStore, simplifyCondStoreSequence,simplifyInstanceOfTestToCheckcastandsimplifyBoundCheckWithThrowException. This change disables those transformations by default, allowing them to be enabled based on the settings of various environment variables. The intention is that they should be more thoroughly tested and then re-enabled as that testing and any necessary bug fixes have been completed.