-
Notifications
You must be signed in to change notification settings - Fork 398
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
Remove unsafe expressions after store sinking #7573
Remove unsafe expressions after store sinking #7573
Conversation
The Store Sinking optimization contain calls to dumpMethodTrees that were explicitly disabled. This hampers debugging of the optimization. This change dumps the method trees before and after the optimization if tracing of the optimization has been enabled. Signed-off-by: Henry Zongaro <[email protected]>
Following is an example that can be used to reproduce the problematic IL in method TestStoreSinking3.java
Compiled with the options
the IL that results for the assignments
in Part of IL for sub prior to General Store Sinking
without the fix, this becomes: Part of IL for sub following General Store Sinking, without the fix
Notice the load of the now uninitialized Part of IL for sub following General Store Sinking, with the fix
@vijaysun-omr, may I ask you to review this change? |
I'll review this and probably will find the answer in the commit moving the pass inside isolated store elimination to its own pass, but I thought I'd ask here anyway, to make it easy for anyone reading the above discussion : what is the definition of safe/unsafe nodes as used in the prior description ? Is it "exception causing" essentially ? |
No, it's any load of a symbol whose definition has been removed/moved past the point of its use, but whose use has remained in place. Uses that might cause an exception allowed the problem to be detected, but this is treating anything that might be loading garbage as unsafe, even if it was only used in an operation that would never result in exception - like an integer add operation, say. |
jenkins build all |
jenkins build riscv |
I am more comfortable with this change, thanks. I have approved. |
If you squash commits, I can run tests again |
The nextStoreWasMoved parameter of sinkStorePlacement is never referenced. This change removes this unused parameter. Signed-off-by: Henry Zongaro <[email protected]>
UnsafeSubexpressionRemoval is currently used in IsolatedStoreElimination where it is defined. The Store Sinking optimization requires the utility provided by UnsafeSubexpressionRemoval, so this change moves the declaration and implementation of that class into there own files. Also, if the reference count of a store that is to be deleted is non- zero, recreate its parent node - which should be a check or compressedRefs node - as a TR::treetop. Signed-off-by: Henry Zongaro <[email protected]>
Store Sinking might leave behind trees that contain commoned loads if necessary to preserve the order of evaluation of operations. It is possible for such trees to contain a load of a symbol that had a value assigned to it in a tree before the current tree, but that was also moved by Store Sinking. Typically such a load would end up being removed by Dead Trees Elimination, but if that optimization fails to run or fails to run to completion, such a load of an uninitialized symbol might be left behind. That can be especially problematic if it is used in an indirect reference. This change checks for such cases and makes use of UnsafeSubexpressionRemover to rework such a tree. It will keep only the parts that are safe to evaluate and vandalize any loads that are unsafe to evaluate. Signed-off-by: Henry Zongaro <[email protected]>
cdda1b1
to
27d1521
Compare
Thanks, @vijaysun-omr! I have squashed the commits. The only difference between the version that you previously reviewed and this version is that I addressed my own review comment. The difference is functionally identical. |
Jenkins build all |
z/OS failure is an infrastructure issue unrelated to this change:
|
Thanks, review comments are addressed. Tests have passed. Merging. |
In the process of moving stores whose results are not needed on all paths, the General Store Sinking optimization will decide whether the tree representing the store should be copied or simply moved. A tree might be copied if nodes in its subtrees are commoned — leaving the part of the tree that represents the value anchored beneath a
TR::treetop
node ensures the order of evaluation is maintained.However, it is possible for a tree that was left in place like this to contain a load of some symbol whose store had preceded the current tree, but that earlier store was also sunk past the current tree. This usually doesn't pose a problem, as the load must have been unneeded at that point, and a subsequent pass of Dead Trees Elimination would ordinarily remove the unneeded uses of the load of the symbol. However, it might happen that Dead Trees Elimination fails to remove that load, as was described in OpenJ9 issue 17515.
Even then the load that was left behind would often not cause a problem — the value would have to be used in a situation where use of an uninitialized symbol might result in a crash. For example, if the result of the load is used as the child of an
arraylength
operation, as one of the operands of anarraycmp
operation, or as the denominator of anidiv
orldiv
operation. It could also cause a problem for garbage collection if the symbol was thought to contain a valid reference.The Isolated Store Elimination optimization deals with the same situation using its
UnsafeSubexpressionRemoval
class.The
UnsafeSubexpressionRemoval
class is used to handle trees that might contain references to symbols that would be unset following a transformation that removed dead stores, anchoring the parts of the tree that are still safe to evaluate and removing those parts that are unsafe to evaluate.This change factors out
UnsafeSubexpressionRemoval
from Isolated Store Elimination, and it adds changes to General Store Sinking to test whether a load of such a dead symbol might remain in a tree that was left behind by Store Sinking, anchoring safe parts and removing unsafe parts.I will add some further examples and details about the IL that will result below.
Fixes eclipse-openj9/openj9#17515
Fixes eclipse-openj9/openj9#20283