Track distinct live commoned loads of locals for Store Sinking - #7844
Conversation
Store sinking tracks which local variables are used in the value assigned to a candidate store, tracks whether a load of a local variable has been commoned, and whether the load might be killed by a store to that local. All this tracking is done on the basis of the index of the local variable. However, it's possible for a local to be loaded before a store to that local, and the value that existed before that store to be used by the candidate for store sinking. In general, there could be several loads and stores to such a local interleaved, with each of the distinct values loaded being used by commoned references in the store that's a candidate for store sinking. This problem was fixed by having LiveVariableInformation keep track of the distinct nodes that load local variables in backwards tree walks, and the number of distinct loads for each local variable. Also removed a redundant copy into *savedLiveCommonedLoads of the result of a call to TR_LiveVariableInformation::liveCommonedLoads(), and removed an unused TR_BitVector satisfiedLiveCommonedLoads. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
|
The following example is taken from eclipse-openj9/openj9#22145. Trees before General Store Sinking and snippets from trace:Notice that Trees following Store Sinking:Notice that the commoned load of Rerunning the test after applying this change, yields Trees before General Store Sinking and snippets from trace:Notice now that upon reaching n231n during the backwards tree walk, the optimization now recognizes that although that is the first instance of that node from the beginning of the method, it is not the first live commoned load of local symbol 23 ( Trees after General Store Sinking |
|
@vijaysun-omr, may I ask you to review this change? |
A set of trace messages in the TR_LiveVariableInformation method, visitTreeForLocals, report during a backwards tree walk whether a node contains the first commoned reference for a load of a sym. The way the messages are currently reported, they might indicate that a node that does not have a symbol reference contains the first commoned reference to a sym. Corrected this to ensure the message only mentions syms for node that are known to contain references to local symbols. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
|
I wonder if another implementation could have just one data structure, i.e. a list of commoned load nodes hung off an array that can be indexed by the local index. That way you may have been able to check the length of the list at the places where you check the "size" currently. The advantage I am thinking of is a minor one here, but I don't feel the change is needed in the PR before I merge it. If you thought about that and feel it won't work for some reason, I am interested. |
|
Jenkins build all |
|
Jenkins build riscv |
|
Risc-v failure appears to be infrastructure-related. It's been failing for all pull request builds recently. |
That's a good point; I hadn't considered that possibility. I could take a look at that as a potential clean-up item. |
|
The sole failure is an infra issue. Merging |
Store sinking tracks which local variables are used in the value assigned to a candidate store, tracks whether a load of a local variable has been commoned, and whether the load might be killed by a store to that local. All this tracking is done on the basis of the index of the local variable.
However, it's possible for a local to be loaded before a store to that local, and the value that existed before that store to be used by the candidate for store sinking. In general, there could be several loads and stores to such a local interleaved, with each of the distinct values loaded being used by commoned references in the store that's a candidate for store sinking.
This problem was fixed by having
LiveVariableInformationkeep track of the distinct nodes that load local variables in backwards tree walks, and the number of distinct loads for each local variable.Also removed a redundant copy into
*savedLiveCommonedLoadsof the result of a call toTR_LiveVariableInformation::liveCommonedLoads(), and removed an unusedTR_BitVectorsatisfiedLiveCommonedLoads.I will add an illustrative example below.
Fixes: eclipse-openj9/openj9#17033