[code-reflection] RFR: Remove bytecode dialect [v4]

[Paul Sandoz]

On Tue, 6 Feb 2024 15:20:35 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> This patch re-implements `BytecodeLift` to lift from bytecode directly to the core dialect
>> and removes `BytecodeInstructionOps` with the bytecode dialect.
>> 
>> Implementation is complete up to the point of passing all current tests.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Code cleanup

Very good, just a few comments.

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeLift.java line 180:

> 178:                 int ni = bcb.instructions.size();
> 179:                 for (int i = 0; i < ni; i++) {
> 180:                     switch (bcb.instructions.get(i)) {

I think it would be beneficial to separate out the switch over an instruction into a separate method, so the loop logic (over blocks and instructions) can be easily read.

In many cases instructions push an operation result onto the stack and we could factor out that code, where we return a non-null result in such cases. This would also make the code easier to read in the switch cases.

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeLift.java line 214:

> 212:                                 if (!operand.type().equals(varType)) {
> 213:                                     local = b.op(CoreOps.var(Integer.toString(lvm), operand));
> 214:                                     locals.put(lvm++, local);

That looks incorrect (probably always was). The slot is reused with a different type so we need to update the existing entry in the map. This likely always connects to how to manage the map with conditional branching. Suggest adding an `@@@` comment for now.

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeLift.java line 240:

> 238:                         }
> 239:                         case ConstantInstruction inst -> {
> 240:                             Op.Result result = b.op(CoreOps.constant(TypeDesc.INT, inst.constantValue()));

Missing support for float, long, and double, and null constants. Switch on type kind?

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeLift.java line 393:

> 391:                                 sb = c.blockMap.get(succ).successor();
> 392:                             }
> 393:                             stack.push(b.op(CoreOps.branch(sb)));

No need to push the result of a terminal operation, since the operation itself also never pushes onto the stack. Likely harmless though, since the stack is localized to each block and such instructions will be the last in the block. See also other cases.

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeLift.java line 417:

> 415:                             BytecodeBasicBlock fslb = bcb.successors.get(0);
> 416:                             BytecodeBasicBlock tslb = bcb.successors.get(1);
> 417:                             stack.push(b.op(CoreOps.conditionalBranch(b.op(cop), c.blockMap.get(tslb).successor(), c.blockMap.get(fslb).successor())));

Break long line.

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

PR Review: https://git.openjdk.org/babylon/pull/16#pullrequestreview-1866008606
PR Review Comment: https://git.openjdk.org/babylon/pull/16#discussion_r1480376798
PR Review Comment: https://git.openjdk.org/babylon/pull/16#discussion_r1480356886
PR Review Comment: https://git.openjdk.org/babylon/pull/16#discussion_r1480351760
PR Review Comment: https://git.openjdk.org/babylon/pull/16#discussion_r1480369363
PR Review Comment: https://git.openjdk.org/babylon/pull/16#discussion_r1480366116