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]>
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. Signed-off-by: Henry Zongaro <[email protected]>
The nextStoreWasMoved parameter of sinkStorePlacement is never referenced. This change removes this unused parameter. 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]>
|
Following is an example that can be used to reproduce the problematic IL in method TestStoreSinking3.javaCompiled with the options the IL that results for the assignments in Part of IL for sub prior to General Store Sinkingwithout the fix, this becomes: Part of IL for sub following General Store Sinking, without the fixNotice 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 |
| // anchored because it's not safe to evaluate. | ||
| // | ||
| child->recursivelyDecReferenceCount(); | ||
| TR::Node *dummyChild = node->setAndIncChild(0, TR::Node::createConstDead(child, TR::Int32, 0xbad1 /* eyecatcher */)); |
There was a problem hiding this comment.
Is an int value for the vandalized case correct to be an TR::Int32 in every case ? I mean, is the node tree even guaranteed to be type correct ?
I ask, since in escape analysis (I believe) vandalization is done in code that is deemed unreachable, whereas here (in store sinking) I don't think we are talking about unreachable code as such.
There was a problem hiding this comment.
That's a good question. The code below recreates the top-level node as either a PassThrough or a treetop node. If the top-level node's reference count was zero, it becomes a treetop, with the vandalized constant as its only child, so that's safe.
However, if the top-level node's reference count was greater than zero, the store might be, as noted by point 2 in the comment above, a commoned child of a CHK node perhaps or a compressedrefs. I'm not sure what will happen during later optimization passes or code generation with something like this.
NULLCHK or compressedrefs
PassThrough
iconst 0xbad1dead
I can rework this to ensure the type of the constant matches whatever type that child node previously represented.
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::treetopnode 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
arraylengthoperation, as one of the operands of anarraycmpoperation, or as the denominator of anidivorldivoperation. 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
UnsafeSubexpressionRemovalclass.The
UnsafeSubexpressionRemovalclass 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
UnsafeSubexpressionRemovalfrom 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