[code-reflection] RFR: Replace the use of Interpreter to create Quoted instance with specialized code [v13]

[Paul Sandoz]

On Sat, 31 May 2025 03:13:12 GMT, Mourad Abbay <mabbay at openjdk.org> wrote:

>> Creating Quoted instance was done by invoking the Interpreter. The Interpreter is too general and if someone could somehow inject any code model we will interpret that as well. This PR replace the use of the interpreter with specialized code.
>
> Mourad Abbay has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add error messages

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4440:

> 4438:     }
> 4439: 
> 4440:     public static FuncOp quoteOp(Op op) {

We need to explicitly throw if the op is not bound i.e. it's not part of any model so we cannot reliably extract it.

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4457:

> 4455:                 Value inputValue = inputOperandsAndCaptures.get(i);
> 4456:                 Value outputValue = b.parameters().get(i);
> 4457:                 if (inputValue.type() instanceof VarType _) {

Suggestion:

                if (inputValue.type() instanceof VarType) {

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4467:

> 4465:                 // Map the entry block of the lambda's ancestor body to the quoted block
> 4466:                 // We are copying lop in the context of the quoted block, the block mapping
> 4467:                 // ensures the use of operands and captured values are reachable when building

Comment needs to be generalized (refers to lambda and lop).

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4479:

> 4477: 
> 4478:     public static OpAndValues quotedOp(FuncOp funcOp) {
> 4479: 

Suggestion:

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4496:

> 4494:         }
> 4495: 
> 4496:         Op op = qop.quotedOp();

What about the checks of the quoted op body? Is does the quoted op constructor have sufficient checks already?

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4501:

> 4499:         List<Block.Parameter> unvisitedParams = new ArrayList<>(fblock.parameters());
> 4500:         for (Op o : ops) {
> 4501:             if (o instanceof VarOp varOp) {

Use a switch statement

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4512:

> 4510:                         throw new IllegalArgumentException("VarOp initial value came from an operation that's not a ConstantOp");
> 4511:                     }
> 4512:                     if (!op.capturedValues().contains(varOp.result())) {

In general could also be used as an operand.

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4531:

> 4529:                 throw new IllegalArgumentException("Block parameter not an operand nor a captured value");
> 4530:             }
> 4531:         }

I think it would be clearer to check up front that all block parameters are used only once to initialize a var op declared in the function's entry block, and further the var op's result is used. This should simplify the checking of all operations but the last two, since the main focus is then on var op initialized by constant op, and you don't need to track unvisited parameters.

I think we should have a generic error message that includes a print out of the model. Being too detailed on the error message requires some careful finessing to make it understandable. Later we could refine to highlight the location in the model that is problematic.

src/jdk.incubator.code/share/classes/jdk/incubator/code/op/CoreOp.java line 4539:

> 4537:     }
> 4538: 
> 4539:     public record OpAndValues (Op op, SequencedSet<Value> operandsAndCaptures) { }

Move declaration above the method.
Suggestion:

    public record OpAndValues(Op op, SequencedSet<Value> operandsAndCaptures) { }

test/jdk/java/lang/reflect/code/TestQuoteOp.java line 20:

> 18:  * @run testng TestQuoteOp
> 19:  */
> 20: public class TestQuoteOp {

We separate test testing invalid quotations, building explicitly invalid quoted models.

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

PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122121458
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122128076
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122122407
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122122570
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122127088
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122128683
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122172172
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122192889
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122123361
PR Review Comment: https://git.openjdk.org/babylon/pull/424#discussion_r2122193636