[code-reflection] RFR: Bytecode round 12 [v3]

[Paul Sandoz]

On Fri, 30 Aug 2024 14:31:13 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> - Added test for operand values dominance revealed a bug in the BytecodeLift.
>> - Fixed BytecodeLift revealed regression in the roundtrip stability.
>> - Roundtrip instability was caused by a bug in the LocalsTypeMapper.
>> 
>> All the above is fixed and the roundtrip stability is restored.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional commit since the last revision:
> 
>   minor corrections

It's good to see the value-based `isDominatedBy`method being used, probably the first time in anger (not much prior testing on that).

How fastidious are you being about round trip stability? Given the following chain of events, where B == bytecode and M == code mode, 

  B1 -> M1 -> B2 -> M2 -> B3 -> M3

Do you want M1, M2 and M3 to be the same or just M2 and M3?

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeGenerator.java line 399:

> 397:         }
> 398: 
> 399:         Set<Block> stopBlocks = new HashSet<>();

If i got this right, we are dealing with an equivalent of an uninitialized variable, of a primitive type, in source that will be assigned in divergent code paths, and the compiler will check it's DA. In the model we currently require the modelling var op be initialized with a default constant value for the primitive type. We don't currently have a concept of an uninitialized value (which might help?), so we have to detect that all loads are dominated by stores, some of which are initializing stores (which detects the uninitialized case or a redundant initialization).

This specific bit of code after the dominated by checks determines if there is a path from `n`'s declaring block to the entry block that does not pass through any of `dom`'s declaring blocks? Why is that is important?

test/jdk/java/lang/reflect/code/bytecode/TestSmallCorpus.java line 171:

> 169:     private void verify(String category, CoreOp.FuncOp func) {
> 170:         OpWriter.CodeItemNamerOption naming = null;
> 171:         for (Body body : func.bodies()) {

It's possible to collapse all the loops by traversing the operations, passing in `naming` as the argument e.g.,

naming = func.traverse(naming, opVisitor((n, op) -> {
    for (Value v : op.operands()) {
        if (!op.result().isDominatedBy(v)) {
            if (n == null) { n = ... }
            ...
        }
    }
    return n;
}));

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

PR Review: https://git.openjdk.org/babylon/pull/219#pullrequestreview-2273428609
PR Review Comment: https://git.openjdk.org/babylon/pull/219#discussion_r1739399511
PR Review Comment: https://git.openjdk.org/babylon/pull/219#discussion_r1739373420