Allow Value Propagation to transform an array store if the value assigned is known not to be a value type #13403
Conversation
hzongaro
added
comp:jit
perf
project:valhalla
|
Marking this as a draft, as the definition of the Although I've marked it as a draft, I don't expect the content of the pull request to change, so it's reasonable review whenever you have time, Vijay @vijaysun-omr and Daryl @0xdaryl. |
|
Marking as ready-for-review now that OMR pull request #6139 has been merged into OpenJ9-OMR. |
|
I'd also appreciate a review from @jdmpapin on this PR. |
|
jenkins test sanity all jdk17,jdk11 |
|
I forgot this change hasn't been merged. Devin @jdmpapin, have you had a chance to review this? |
There was a problem hiding this comment.
There's one (I think) important comment about the non-null constraint condition, and my other comments are pretty minor
I left in a couple of comments that were addressed in the next commit just to show my thought process reading each commit in turn. IMO series should generally be structured to avoid introducing and then fixing a problem. Just pointing it out - not that you necessarily need to revise this series for that
hzongaro
force-pushed
the
check-aastore-value-is-VT-PR
branch
from
431245f
to
04f40a8
Compare
|
Thanks for reviewing, Devin @jdmpapin. I've pushed commits e70d3eb23bba7b842ac81bf859799b8c380ee443 and 04f40a818c05a06187c94bbc2ed6e5e5f627af4d with the necessary changes. |
Define a new method to find or create a symbol reference for a non- helper method that will be used to check whether an array is a value-types array, and if so, whether a null value is being stored to one of its elements. If so, a NullPointerException will be thrown. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
If a value that is to be stored to an array element via a call to the jitStoreFlattenableArrayElement helper is not a value type, value propagation can transform it to use inline code. If the array has a value type as its component type, an ArrayStoreCheckException or a NullPointerException will result, so there is no need to use the helper to handle the possibility of a flattenable array. The test is represented in the IL with a call to the nonNullableArrayStoreNullCheck non-helper, which is expanded in the Tree Lowering optimization into an actual test of the whether the component type of the array is a value type guarding a NULLCHK of the value. This renders obsolete prototype code that overloaded the ArrayStoreCHK opcode to perform this additional checking, so logic Tree Lowering for ArrayStoreCHK is also removed. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
When generating inline IL for nonNullableArrayNullStoreCheck in the Tree Lowering optimization, rather than checking whether the component type of an array is a value type and then checking whether the value being stored is null, it should typically be faster to check first whether the value is null. That avoids the double indirection to access the class flags of the component type if the value that's being assigned is non- null. Also added missing edges to CFG for branches around NULLCHK. Authored-by: Annabelle Huo <Annabelle.Huo@ibm.com> Co-authored-by: Henry Zongaro <zongaro@ca.ibm.com> Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
If Value Propagation or Tree Lowering generate BNDCHK or ArrayStoreCHK operations, the alias sets for the checking symbol reference might not have been constructed. Mark alias sets as invalid so they can be rebuilt if they are needed after those optimizations. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
When Value Propagation determines that a call to <jitStoreFlattenableArrayElement> can be transformed due to constraints associated with the value that is being stored, it should determine whether the value is known to be non-null by checking the constraint associated with the value rather than calling isNonNull() on the node. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
hzongaro
force-pushed
the
check-aastore-value-is-VT-PR
branch
from
04f40a8
to
7f95a3b
Compare
Thanks! Squashed less interesting commits. |
|
Vijay @vijaysun-omr, I just wanted to check whether we're OK to move forward on this one. |
|
Yes I am fine with this series of commits now. |
|
Jenkins test sanity xlinuxval,xlinuxvalst jdk17 |
|
Jenkins test sanity xlinuxval,xlinuxvalst jdknext |
|
Jenkins test sanity all jdk17 |
|
@0xdaryl, would you mind taking a look since your review was requested above? |
If the value stored in a call to
jitStoreFlattenableArrayElementis known not to be a value type, the call can be transformed by Value Propagation to inline IL for an ordinary array store even if the component type of the array is not known not to be a value type. That's because, if the component type of the array actually is a value type, theArrayStoreCHKadded as part of the array element store sequence would throw anArrayStoreException- there is no need to deal with a potential store to a flattened value type array element.The one special case that needs to be handled in the inline IL that's generated is the case where the value that is being stored could be a null reference. The prototype support for value types does not permit a null reference to be stored into an element of a value type array, and
ArrayStoreCHKnever throws an exception if the value is a null reference. A call to a non-helper symbol,<nonNullableArrayNullStoreCheck>, is added to the IL to handle that case. During the Tree Lowering optimization, the call to that non-helepr is expanded to IL that will throw aNullPointerExceptionif the value is a null reference and the component type of the array is a value type.Some older prototype code in Tree Lowering that overloaded
ArrayStoreCHKto do this special handling to check for a null reference is now redundant, and removed.Delivering these changes on behalf of Annabelle Huo @a7ehuo and myself.