[code-reflection] RFR: Aggregated work on Verifier and BytecodeLift deconstruction

[Adam Sotona]

On Tue, 5 Nov 2024 18:04:33 GMT, Paul Sandoz <psandoz at openjdk.org> wrote:

>> This PR is aggregated continuation of work on `Verifier` #247 and new simplified and deconstructed implementation of `BytecodeLift`.
>> 
>> Work on `Verifier` revealed bugs in `BytecodeLift`.
>> Fixing of the bugs adds more complexity to already very complex `BytecodeLift`.
>> The goal of this PR is to deconstruct `BytecodeLift` to its bare bones  and re-introduce a kind of bytecode dialect:
>> - `UnresolvedType.Ref` and `UnresolvedType.Int` represent references and int with all its possible sub-types
>> - `SlotOp` is an intermediate model of local variables
>> - `SlotToVarTransformer` transforms `SlotOp` to `CoreOp.VarOp`
>> - `UnresolvedTypesTransformer` transforms `UnresolvedType.Ref` and `UnresolvedType.Int` to `JavaType`.
>
> src/java.base/share/classes/java/lang/reflect/code/Value.java line 152:
> 
>> 150:      * @throws IllegalStateException if the declaring block is partially built
>> 151:      */
>> 152:     public boolean isDominatedBy(Set<? extends Value> doms) {
> 
> It would be better for now to move this method back into the bytecode area as non-public. Currently it's too specialized and not-well defined.
> 
> From reading the specification one might expect it to return true if this value is dominated be all given values. But, it returns true if any given value does. If none dominate then it checks if there is path back from this value's block to the entry block of the same parent body. That is not directly related to dominance, but related dominance frontiers which is a different concept. Returning true in the second cases doe not mean dominance.

This method tests if the given set of values represent a dominating set for the actual value. I hope the implementation matches the "Dominating sets in directed graphs" definition.

I've moved it here because there are already two uses of the method:
- BytecodeGenerator::isDefinitelyAssigned needs to test for the dominance of VarStoreOps set to decide if the VarOp can be deferred 
- SlotToVarTransformer::isDominatedByTheSameVar needs to test dominance of set of SlotOp predecessors to identify initial SlotStoreOps.

> src/java.base/share/classes/java/lang/reflect/code/bytecode/UnresolvedType.java line 51:
> 
>> 49:         private static final TypeElement.ExternalizedTypeElement UNRESOLVED_REF = new TypeElement.ExternalizedTypeElement("?REF", List.of());
>> 50: 
>> 51:         private JavaType resolved;
> 
> Sneaky mutability :-) Not something i would recommend although i understand the convenience. You could instead use a map of unresolved type to resolved type. But let's consider that later as i think there is a broader issue. More generally the transformation of types is harder than it should be, and this is a good use case to help improve this area. I don't have any solid ideas right now but hopefully we can eventually come up with something better.

Yes, this is not ideal. Maps can be used later in the transform, however unresolved component of an unresolved array type is something that needs to be at least partially resolved (2-slot long or double vs 1-slot others) in the first stage of BytecodeLift or it causes stack problems with hybrid single and dual slot instructions (dup_x2, dup2, dup2_x1 and dup2_x2). The problem here is that I cannot retrospectively change types in the middle of a lift process. 

I'll definitelly look for better solution.

-------------

PR Review Comment: https://git.openjdk.org/babylon/pull/258#discussion_r1830876466
PR Review Comment: https://git.openjdk.org/babylon/pull/258#discussion_r1830893450