[code-reflection] RFR: Test remove final vars

[Paul Sandoz]

On Thu, 15 Feb 2024 10:08:38 GMT, Mourad Abbay <mabbay at openjdk.org> wrote:

> A transformation to remove final vars, it serves as an example on how to use the API to write transformations.

test/jdk/java/lang/reflect/code/TestRemoveFinalVars.java line 39:

> 37: 
> 38:         FuncOp f2 = f.transform(TestRemoveFinalVars::rmFinalVars);
> 39:         f2.writeTo(System.out);

We should assert on the resulting model or run through the interpreter. I suspect the result of your transformation would result in the same structure as the one produced by `SSA.transform`.

test/jdk/java/lang/reflect/code/TestRemoveFinalVars.java line 43:

> 41: 
> 42:     /*
> 43:     if VarOp

IMO you can drop this comment and embedded within the transform code.

test/jdk/java/lang/reflect/code/TestRemoveFinalVars.java line 53:

> 51:     static Block.Builder rmFinalVars(Block.Builder block, Op op) {
> 52:         if (op instanceof VarOp varOp) {
> 53:             if (isValueUsedWithOpClass(varOp.result(), VarStoreOp.class)) {

Since this is an example we can improve this (since we might generalize on interfaces implementation by ops) by passing in a Predicate<Op> e.g., `op -> op instanceof VectorStoreOp` (then one can layer on top class equality as a helper function if needed).

test/jdk/java/lang/reflect/code/TestRemoveFinalVars.java line 53:

> 51:     static Block.Builder rmFinalVars(Block.Builder block, Op op) {
> 52:         if (op instanceof VarOp varOp) {
> 53:             if (isValueUsedWithOpClass(varOp.result(), VarStoreOp.class)) {

Add comment:

// Is the variable stored to? If not we can remove it
// otherwise, it's not considered final and we copy it

test/jdk/java/lang/reflect/code/TestRemoveFinalVars.java line 57:

> 55:             }
> 56:         } else if (op instanceof VarLoadOp varLoadOp) {
> 57:             if (!isValueUsedWithOpClass(varLoadOp.varOp().result(), VarStoreOp.class)) {

Comment:
// If the variable is not stored to

test/jdk/java/lang/reflect/code/TestRemoveFinalVars.java line 59:

> 57:             if (!isValueUsedWithOpClass(varLoadOp.varOp().result(), VarStoreOp.class)) {
> 58:                 CopyContext cc = block.context();
> 59:                 cc.mapValue(varLoadOp.result(), cc.getValue(varLoadOp.varOp().operands().get(0)));

Let's add a comment here:

// Map result of load from variable to the value that initialized the variable
// Subsequently encountered input operations using the result will be copied 
// to output operations using the mapped value

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

PR Review Comment: https://git.openjdk.org/babylon/pull/25#discussion_r1491530668
PR Review Comment: https://git.openjdk.org/babylon/pull/25#discussion_r1491526475
PR Review Comment: https://git.openjdk.org/babylon/pull/25#discussion_r1491518060
PR Review Comment: https://git.openjdk.org/babylon/pull/25#discussion_r1491521479
PR Review Comment: https://git.openjdk.org/babylon/pull/25#discussion_r1491522663
PR Review Comment: https://git.openjdk.org/babylon/pull/25#discussion_r1491520189