(0.31.0) Replace forceUnresolvedDispatch() with isResolved*DispatchGuaranteed() #14463
Conversation
This logic is identical between TR_ResolvedRelocatableJ9Method and TR_ResolvedRelocatableJ9JITServerMethod. It is about to be updated in a later commit, so this refactoring will ensure that the two remain consistent.
Certain transformations in the compiler rely on a guarantee that a given resolved call node will simply call the target method without attempting to resolve a constant pool entry, since the constant pool entry may not correspond to the call in the usually expected way. We always have this guarantee in non-AOT compilations. In AOT compilations, we currently have this guarantee for direct calls when using the symbol validation manager (SVM) on x86, Power, and z. In the SVM case, each code generator must now opt in to this class of transformations by explicitly declaring that it provides the needed guarantee for direct calls, virtual calls, or both. Special-case virtual and interface resolution in AOT now respects isResolved*DispatchGuaranteed(). In particular, private invokevirtual, private invokeinterface, and final Object invokeinterface will all become direct, so heed isResolvedDirectDispatchGuaranteed(), and non-final Object invokeinterface will become virtual, so now heeds isResolvedVirtualDispatchGuaranteed(). Additionally, refinement of invokeBasic() and linkTo*() now also respects these queries, though it shouldn't (currently) be attempted in AOT regardless. Refinement requires a known object index for either the MethodHandle or the MemberName, and AOT does not support the known object table. It probably also makes sense to gate value propagation's type refinement of indirect calls on isResolvedDirectDispatchGuaranteed() for fixed type and isResolvedVirtualDispatchGuaranteed() for type bounds, but this is not done in this commit. That change would require modifications to OMR, and there is currently no problem with call refinement in VP because it respects canDevirtualizeDispatch(), which is false in AOT compilations. As a result of the above, this commit effectively stops AOT compilations using the SVM from generating resolved virtual calls for invokeinterface calling non-final methods of Object. Instead, such calls are left to be treated as (unresolved) interface calls as in non-SVM AOT. This ilgen transformation can be re-enabled for a particular code generator by overriding guaranteesResolvedVirtualDispatchForSVM() to return true, but first it will be necessary to ensure that the code generator really provides that guarantee. If the sequence generated for a resolved virtual call is simply a call through the vtable, then providing the guarantee should only involve: - sending execution to the resolved path when the SVM is enabled, and - generating an appropriate J2I thunk validation. However, the required J2I thunk validation does not yet exist. It will be implemented separately.
|
Let's wait one more day for things to settle. |
|
For jdk18 #14465 was opened. Not sure if it's related, probably not. The failures aren't new, they've occurred for some time as seen in #13946. We just didn't know if they would be fixed by this change or not. In the head stream builds for jdk8, 11, 17, the following new issues were opened on the builds last night, both on AIX. |
|
Nothing else in the 8, 11, 17 builds last night, just unrelated intermittent problems we've seen before. |
|
Is the concern with merging this that #14469 and #14470 might be regressions caused by it? If so, @IBMJimmyk do you have any insight into those failures yet? Could we revert this PR in a private build and grind these failing tests to confirm? Given the number of failures this PR fixes then I don't want to keep it out of the builds for too much longer. |
|
I haven't really gotten started in the investigation into #14469 yet so I don't have anything to add at this time. |
|
I don't have any concern with merging this, I think we should go ahead with the merge, and then do a Milestone 1 for 0.31 while further investigation continues. |
|
Agreed. @dsouzai, please merge if you have no objections since this is assigned to you. |
This is the v0.31.0-release PR corresponding to #14430. The commit to be merged here is the identical commit that was already merged into master as part of the original PR