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

[Paul Sandoz]

On Fri, 18 Oct 2024 10:06:38 GMT, Adam Sotona <asotona 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`.

The general approach to layering the lifting looks good. Just a few comments, mostly related to the impact outside of the bytecode package.

src/java.base/share/classes/java/lang/reflect/code/TypeElement.java line 114:

> 112:     ExternalizedTypeElement externalize();
> 113: 
> 114:     default TypeElement componentType() {

I think it would be better to introduce new interface e.g., `TypeWithComponent` then we can test or pattern match on that.

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.

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.

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

PR Review: https://git.openjdk.org/babylon/pull/258#pullrequestreview-2416328120
PR Review Comment: https://git.openjdk.org/babylon/pull/258#discussion_r1829777860
PR Review Comment: https://git.openjdk.org/babylon/pull/258#discussion_r1829798524
PR Review Comment: https://git.openjdk.org/babylon/pull/258#discussion_r1829891446