Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support storing the code that builds the code model #305

Closed
wants to merge 25 commits into from
+501 −27
Closed

Support storing the code that builds the code model #305

Changes from 15 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0d53ba1
Add option to indicate how to store code model
mabbay Jan 22, 2025
4cdc021
Make TEXT the default storage mechanism for code models
mabbay Jan 22, 2025
c3a71f9
Support storing code that builds code model
mabbay Feb 10, 2025
132fb5f
Add JavaType -> Type mapping for Block.Parameter
mabbay Feb 10, 2025
d4d3a25
Add a program to test storing code that builds code model
mabbay Feb 10, 2025
16b1608
Define parsing code in the enum CodeModelStorageOption
mabbay Feb 10, 2025
54e4c30
Apply fixes from Maurizio
mabbay Feb 11, 2025
21c12cb
Reformat code
mabbay Feb 11, 2025
86a3959
Support storing code that builds code model
mabbay Feb 23, 2025
cd302fc
Add comment
mabbay Feb 23, 2025
f832790
Merge branch 'code-reflection' into code-model-storage-option
mabbay Feb 24, 2025
e9fcd8c
Add missing imports
mabbay Feb 25, 2025
c232366
Ensure that block params are inserted in the correct order
mabbay Feb 26, 2025
a1d134c
Pass arrayType instead of eleType in OpBuilder.buildArray
mabbay Feb 28, 2025
5f02f73
Fix almost all test failures of SwitchExpressionTest2 (one remaining)
mabbay Mar 1, 2025
c17be6b
Fix the remaining test failures of SwitchExpressionTest2
mabbay Mar 3, 2025
cd143c3
Fix some of the test failures (3 remains)
mabbay Mar 3, 2025
ee0f906
Fix the remaining compiler tests failures
mabbay Mar 6, 2025
0e49cce
Load OpFactory and TypeElementFactory before invocation of opMethod o…
mabbay Mar 6, 2025
966005c
Merge branch 'code-reflection' into code-model-storage-option
mabbay Mar 12, 2025
95f227e
Don't rely on the assumption of 1d array
mabbay Mar 19, 2025
2e30920
Make opMethod descriptor consistent for both storage options.
mabbay Mar 26, 2025
bf96a44
Changes in code due to making opMethod signature uniform
mabbay Mar 27, 2025
f9432af
Make return type of opMethod the same for all storage options
mabbay Mar 28, 2025
3e3095c
Merge branch 'code-reflection' into code-model-storage-option
mabbay Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/jdk.incubator.code/share/classes/jdk/incubator/code/Op.java
View file
Original file line number Diff line number Diff line change
@@ -40,8 +40,12 @@
import com.sun.tools.javac.util.Context;
import jdk.incubator.code.internal.ReflectMethods;
import jdk.incubator.code.op.CoreOp.FuncOp;
import jdk.incubator.code.op.ExtendedOp;
import jdk.incubator.code.op.OpFactory;
import jdk.incubator.code.type.CoreTypeFactory;
import jdk.incubator.code.type.FunctionType;
import jdk.incubator.code.type.MethodRef;
import jdk.incubator.code.type.TypeElementFactory;
import jdk.incubator.code.writer.OpWriter;
import jdk.internal.access.SharedSecrets;

@@ -526,17 +530,25 @@ private static Optional<FuncOp> createCodeModel(Method method) {
case '.', ';', '[', '/': sig[i] = '$';
}
}
String opMethodName = "op$" + new String(sig);
String opMethodName = new String(sig);
Method opMethod;
Object[] args;
try {
// @@@ Use method handle with full power mode
opMethod = method.getDeclaringClass().getDeclaredMethod(opMethodName);
args = new Object[] {};
} catch (NoSuchMethodException e) {
return Optional.empty();
try {
opMethod = method.getDeclaringClass().getDeclaredMethod(opMethodName, OpFactory.class,
TypeElementFactory.class);
args = new Object[] {ExtendedOp.FACTORY, CoreTypeFactory.CORE_TYPE_FACTORY};
} catch (NoSuchMethodException e2) {
return Optional.empty();
}
}
opMethod.setAccessible(true);
try {
FuncOp funcOp = (FuncOp) opMethod.invoke(null);
FuncOp funcOp = (FuncOp) opMethod.invoke(null, args);
return Optional.of(funcOp);
} catch (ReflectiveOperationException e) {
throw new RuntimeException(e);
287 changes: 287 additions & 0 deletions src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/CodeModelToAST.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,287 @@
package jdk.incubator.code.internal;

import com.sun.tools.javac.code.*;
import com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.tree.TreeMaker;
import com.sun.tools.javac.util.*;
import jdk.incubator.code.*;
import jdk.incubator.code.op.CoreOp;
import jdk.incubator.code.op.ExternalizableOp;
import jdk.incubator.code.op.OpFactory;
import jdk.incubator.code.type.*;

import java.lang.invoke.MethodHandles;
import java.lang.reflect.Method;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;

import static com.sun.tools.javac.code.Flags.*;

public class CodeModelToAST {

private final TreeMaker treeMaker;
private final Names names;
private final Symtab syms;
private final Symbol.ClassSymbol currClassSym;
private final CodeReflectionSymbols crSym;
private final Map<Value, JCTree> valueToTree = new HashMap<>();
private final Map<JavaType, Type> jtToType;
private Symbol.MethodSymbol ms;

public CodeModelToAST(TreeMaker treeMaker, Names names, Symtab syms,
Symbol.ClassSymbol currClassSym, CodeReflectionSymbols crSym) {
this.treeMaker = treeMaker;
this.names = names;
this.syms = syms;
this.currClassSym = currClassSym;
this.crSym = crSym;
this.jtToType = mappingFromJavaTypeToType();
}

private Map<JavaType, Type> mappingFromJavaTypeToType() {
Map<JavaType, Type> m = new HashMap<>();
Symbol.ModuleSymbol jdk_incubator_code = syms.enterModule(names.jdk_incubator_code);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is effectively an extension to javac's symbol table. My preference would be to:

  • add whatever symbol/type you need in CodeReflectionSymbol
  • then set up the translation map simply, like you do for primitive types -- e.g.
Map.entry(<type element>, <javac type>)

Class<?>[] crTypes = {Body.Builder.class, TypeElement.ExternalizedTypeElement.class, TypeElement.class,
FunctionType.class, Block.Builder.class, Value.class, Block.Reference.class, Op.Result.class,
Op.class, TypeElementFactory.class, OpFactory.class, ExternalizableOp.ExternalizedOp.class,
MethodRef.class, Block.Parameter.class, FieldRef.class, CoreOp.InvokeOp.InvokeKind.class,
ExternalizableOp.class, RecordTypeRef.class
};
for (Class<?> crType : crTypes) {
JavaType jt = JavaType.type(crType.describeConstable().get());
Type t = syms.enterClass(jdk_incubator_code, jt.externalize().toString());
m.put(jt, t);
}
Class<?>[] javaBaseTypes = {HashMap.class, String.class, Object.class, Map.class, java.util.List.class};
for (Class<?> javaBaseType : javaBaseTypes) {
JavaType jt = JavaType.type(javaBaseType.describeConstable().get());
Type t = syms.enterClass(syms.java_base, jt.externalize().toString());
m.put(jt, t);
}

m.putAll(Map.ofEntries(
Map.entry(JavaType.BOOLEAN, syms.booleanType),
Map.entry(JavaType.BYTE, syms.byteType),
Map.entry(JavaType.SHORT, syms.shortType),
Map.entry(JavaType.CHAR, syms.charType),
Map.entry(JavaType.INT, syms.intType),
Map.entry(JavaType.LONG, syms.longType),
Map.entry(JavaType.FLOAT, syms.floatType),
Map.entry(JavaType.DOUBLE, syms.doubleType)
));

return m;
}

private Type typeElementToType(TypeElement te) {
JavaType jt = (JavaType) te;
return switch (jt) {
case ClassType ct when ct.hasTypeArguments() -> {
ClassType enclosingType = ct.enclosingType().orElse(null);
ListBuffer<Type> typeArgs = new ListBuffer<>();
for (JavaType typeArgument : ct.typeArguments()) {
typeArgs.add(typeElementToType(typeArgument));
}
yield new Type.ClassType(typeElementToType(enclosingType), typeArgs.toList(),
typeElementToType(ct.rawType()).tsym);
}
case ArrayType at -> {
yield new Type.ArrayType(typeElementToType(at.componentType()), syms.arrayClass);
}
case null -> Type.noType;
default -> {
if (!jtToType.containsKey(te)) {
throw new IllegalStateException("JavaType -> Type not found for: " + te.externalize().toString());
}
yield jtToType.get(te);
}
};
}

private JCTree invokeOpToJCMethodInvocation(CoreOp.InvokeOp invokeOp) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has some issues -- but perhaps we can keep it for now.
The main problem is that we seem to copy types from the invoked method declaration and put them "as is" in the generated AST. This works only as long as the invoked method doesn't have any generic type. For instance, consider a call to List::add. The signature of this method accepts X which is a type parameter of List<X>. I believe the code you have copies the X from the declared signature and sticks it into the javac's MethodType instance attached to the method call. Since this method type is used to describe the use-site (the call), and not the declaration, the type seems incorrect. E.g. calling List<String>:add should have a type of add(String) not add(X). (btw, a similar problem is present for field access, if the type of the accessed field is a type-variable -- so it's not just an issue with method calls).

The type of the invoked method/accessed field should always be adjusted with Types.memberType first. This method takes two parameters:

  • the receiver type you are using to call the instance method/access the instance field
  • the symbol you are trying to access

And returns the instantiated type of that symbol at that receiver type. Examples:

memberType(List<String>, add(X)->void) -> (String)->void
memberType(List<Integer>, add(X)->void) -> (Integer)->void

Once you address this, there is still another problem with generic methods -- as methods can have their own type-variables too. To figure out what is the type to be replaced for these type-variables you generally need to run inference -- which seems way too much for what we're trying to do here. The issue here is that the code model you are processing doesn't expose these details, and so generating the AST needs to "trace back" whatever steps where done when generating this model -- which is an hard problem.

I hope we can get away with just working with erased types, and maybe insert type-conversion to convert the type of the invoked method/accessed field so that it matches the expected op result type.

Method method;
try {
method = invokeOp.invokeDescriptor().resolveToDirectMethod(MethodHandles.lookup());
} catch (ReflectiveOperationException e) {
throw new RuntimeException(e);
}
long flags = method.getModifiers();
var name = names.fromString(invokeOp.invokeDescriptor().name());
Value receiver = invokeOp.invokeKind() == CoreOp.InvokeOp.InvokeKind.INSTANCE ? invokeOp.operands().get(0) : null;
List<Value> arguments = invokeOp.operands().stream().skip(receiver == null ? 0 : 1).collect(List.collector());
var paramTypes = invokeOp.invokeDescriptor().type().parameterTypes().stream().map(this::typeElementToType)
.collect(List.collector());
var restype = typeElementToType(invokeOp.resultType());
var type = new Type.MethodType(paramTypes, restype, List.nil(), syms.methodClass);
var methodSym = new Symbol.MethodSymbol(flags, name, type,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for field access (see below) -- we should not create a new method symbol from scratch but "resolve" a method in a given class symbol (using Resolve::findInternalMethod)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to add two helper methods -- one takes a FieldDescriptor and returns a VarSymbol (using Resolve::findFieldInternal -- the other takes a MethodDescriptor and returns a MethodSymbol (using Resolve::findMethodIntenal. Then you rewrite the code here (and in field access) to use these methods.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to the one above for var symbol -- creating method/var symbols directly in the backend is not the way to do things -- we should lean on Resolve instead. It would be nice to fix these -- but we can also take another pass at it later if required.

typeElementToType(invokeOp.invokeDescriptor().refType()).tsym);
var meth = receiver == null ? treeMaker.Ident(methodSym) : treeMaker.Select((JCTree.JCExpression) valueToTree.get(receiver), methodSym);
var args = new ListBuffer<JCTree.JCExpression>();
for (Value operand : arguments) {
args.add((JCTree.JCExpression) valueToTree.get(operand));
}
var methodInvocation = treeMaker.App(meth, args.toList());
if (invokeOp.isVarArgs()) {
var lastParam = invokeOp.invokeDescriptor().type().parameterTypes().getLast();
Assert.check(lastParam instanceof ArrayType);
methodInvocation.varargsElement = typeElementToType(((ArrayType) lastParam).componentType());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problem as described above -- here we're copying the vararg array at the declaration site into a type at the use-site -- for something like List::of this won't give the result you expect.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now -- while all the points I made above apply (e.g. if you were to try and type-check the generated AST using Attr you will get several errors), the saving grace here is that you are sending this tree into javac's backend anyway. And the backend plays a lot looser with types, only inserting casts where absolutely needed. Since you are using the op result type on the MethodType instance you are generating, I believe that should be enough for the backend to at least insert 90% of the type conversions that might be required because of erasure. So, in practice, even if incorrect, the code above might work fine.

}
if (invokeOp.result().uses().isEmpty()) {
return treeMaker.Exec(methodInvocation);
}
return methodInvocation;
}

private JCTree opToTree(Op op) {
JCTree tree = switch (op) {
case CoreOp.ConstantOp constantOp when constantOp.value() == null ->
treeMaker.Literal(TypeTag.BOT, null).setType(syms.botType);
case CoreOp.ConstantOp constantOp -> treeMaker.Literal(constantOp.value());
case CoreOp.InvokeOp invokeOp -> invokeOpToJCMethodInvocation(invokeOp);
case CoreOp.NewOp newOp -> {
if (newOp.resultType() instanceof ArrayType at) {
var elemType = treeMaker.Ident(typeElementToType(at.componentType()).tsym);
// @@@ we assume one dimension
var dims = List.of((JCTree.JCExpression) valueToTree.get(newOp.operands().getFirst()));
var na = treeMaker.NewArray(elemType, dims, null);
na.type = typeElementToType(at);
yield na;
}
var ownerType = typeElementToType(newOp.constructorType().returnType());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a strange asymmetry in the model between constructors and methods. Methods have a descriptor, constructors do not. Also, for methods you know from the op if it is varargs, but for constructors this info is not available. My feeling is that (at least coming at this from javac), trying to lump array and class creation under one op is probably why these asymmetries crop up.

This has nothing to do with this PR of course -- but I suspect that if you had a varargs constructor call this code would not work.

Copy link
Member Author

@mabbay mabbay Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on all points. We can have a new.array operation and then model class creation as an invoke op with method name being "init" ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on all points. We can have a new.array operation and then model class creation as an invoke op with method name being "init" ?

not sure I follow -- I was talking about class construction -- arrays are a bit special (e.g. there's no array constructor descriptor, unlike for classes). But a constructor call does share a lot of similarities with a method call, and there should be reusable code between method and constructors -- as it happens inside the javac code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be deferred until we have a better op to model instance creation expression.

var clazz = treeMaker.Ident(ownerType.tsym);
var args = new ListBuffer<JCTree.JCExpression>();
for (Value operand : newOp.operands()) {
args.add((JCTree.JCExpression) valueToTree.get(operand));
}
var nc = treeMaker.NewClass(null, null, clazz, args.toList(), null);
var argTypes = new ListBuffer<Type>();
for (Value operand : newOp.operands()) {
argTypes.add(typeElementToType(operand.type()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong -- and is related to the point I made above -- the type of the operands here are the types of the actual arguments of the constructor calls. So, if you have:

class Foo {
     Foo(String string) { ... }
 }

the method type of the constructor call has to say Foo(String). But if you "infer" the type of the constructor argument from the actual argument, if the call is like this:

new Foo(null)

We should probably use the op's constructorType to derive the type of the invoked constructor? (You use that to determine the constructor's owner type)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be deferred until we have a better op to model instance creation expression.

}
nc.type = ownerType;
nc.constructorType = new Type.MethodType(argTypes.toList(), syms.voidType, List.nil(), syms.methodClass);
nc.constructor = new Symbol.MethodSymbol(PUBLIC, names.init, nc.constructorType, ownerType.tsym);
yield nc;
}
case CoreOp.ReturnOp returnOp ->
treeMaker.Return((JCTree.JCExpression) valueToTree.get(returnOp.returnValue()));
case CoreOp.VarOp varOp when varOp.initOperand() instanceof Block.Parameter p -> valueToTree.get(p);
case CoreOp.VarOp varOp -> {
var name = names.fromString(varOp.varName());
var type = typeElementToType(varOp.varValueType());
var v = new Symbol.VarSymbol(LocalVarFlags, name, type, ms);
yield treeMaker.VarDef(v, (JCTree.JCExpression) valueToTree.get(varOp.initOperand()));
}
case CoreOp.VarAccessOp.VarLoadOp varLoadOp
when varLoadOp.varOp().initOperand() instanceof Block.Parameter p2 -> valueToTree.get(p2);
case CoreOp.VarAccessOp.VarLoadOp varLoadOp ->
treeMaker.Ident((JCTree.JCVariableDecl) valueToTree.get(varLoadOp.varOperand()));
case CoreOp.FieldAccessOp.FieldLoadOp fieldLoadOp -> {
// Type.fieldName
// if instance field we will use the same thechnique as in invokeOpToTree
int flags;
try {
flags = fieldLoadOp.fieldDescriptor().resolveToMember(MethodHandles.lookup()).getModifiers();
} catch (ReflectiveOperationException e) {
throw new RuntimeException(e);
}
var name = names.fromString(fieldLoadOp.fieldDescriptor().name());
var type = typeElementToType(fieldLoadOp.resultType());
var owner = typeElementToType(fieldLoadOp.fieldDescriptor().refType());
var sym = new Symbol.VarSymbol(flags, name, type, owner.tsym);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong -- the accessed field symbol should be findable in the scope of the class in which the field is declared. It seems to me that we need some piece of code that turns a method/field descriptor into a java symbol. Look at Lower::lookupMethod which uses Resolve::resolveInternalMethod -- I think here we should do that too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is borderline, but again, we can leave it as is for now (the risk is that if we have a bad model javac will just emit a reference to some non-existent field, which will likely cause non-sensical bytecode to be emitted).

yield treeMaker.Select(treeMaker.Ident(owner.tsym), sym);
}
case CoreOp.ArrayAccessOp.ArrayStoreOp arrayStoreOp -> {
var array = arrayStoreOp.operands().get(0);
var val = arrayStoreOp.operands().get(1);
var index = arrayStoreOp.operands().get(2);
var as = treeMaker.Assign(
treeMaker.Indexed((JCTree.JCExpression) valueToTree.get(array), (JCTree.JCExpression) valueToTree.get(index)),
(JCTree.JCExpression) valueToTree.get(val)
);
yield treeMaker.Exec(as);
// body builder are created but never passed when creating the op, why ?
}
default -> throw new IllegalStateException("Op -> JCTree not supported for :" + op.getClass().getName());
};
valueToTree.put(op.result(), tree);
return tree;
}

public JCTree.JCMethodDecl transformFuncOpToAST(CoreOp.FuncOp funcOp, Name methodName) {
Assert.check(funcOp.body().blocks().size() == 1);

var paramTypes = List.of(crSym.opFactoryType, crSym.typeElementFactoryType);
var mt = new Type.MethodType(paramTypes, crSym.opType, List.nil(), syms.methodClass);
ms = new Symbol.MethodSymbol(PUBLIC | STATIC | SYNTHETIC, methodName, mt, currClassSym);
currClassSym.members().enter(ms);

// TODO add VarOps in OpBuilder
funcOp = addVarsWhenNecessary(funcOp);
funcOp.writeTo(System.out);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed?

Copy link
Member Author

@mabbay mabbay Mar 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addVarsWhenNecessary is used to add necessary VarOp in the code model before transformation to AST. For example if an op result is used more than once, we introduce a VarOp to hold the value, because that's what the equivalent Java code will have. This makes the transformation from code model to AST a bit easier.
We also use it to in case we want an Op (e.g. InvokeOp) to be appended right away to the method we are building. As you now the block of a method is a list of statements, some operations maps to an expression, so they won't be appended until needed. So by putting the result of an op in a VarOp, that VarOp will be mapped to a statement which causes the tree of the op to be added immediately.
For example an InvokeOp to Block.Builder::op is something we want to be transformed to a JCTree and added immediately to the method's block, so that we preserve the order of the operations as in the original code model.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, the OpBuilder should produces a model that contains the VarOp when they should be. But I decided to have a seperate transformation as a quick fix. Later I will change the OpBuilder.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies -- I meant whether the writeTo should be removed :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I decided to have a seperate transformation as a quick fix. Later I will change the OpBuilder.

As stated in another comment, I'm not sure changing the op builder is needed -- as the javac code can easily workaround what the op builder does (e.g. what it does doesn't seem obviously wrong to me). But if the decision is to fix op builder that's fine too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to directly avoid changing OpBuilder (investing in a SSA to Var transformation, using liveness analysis, is probably the general right approach), but there might be a more focused approach to generating the AST so we can avoid this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming @PaulSandoz -- we can defer this improvement to a follow up change -- and keep the var-adding transformation in this PR for now.

for (int i = 0; i < funcOp.parameters().size(); i++) {
valueToTree.put(funcOp.parameters().get(i), treeMaker.Ident(ms.params().get(i)));
}

var stats = new ListBuffer<JCTree.JCStatement>();
for (Op op : funcOp.body().entryBlock().ops()) {
var tree = opToTree(op);
if (tree instanceof JCTree.JCStatement stat) {
stats.add(stat);
}
}
var mb = treeMaker.Block(0, stats.toList());

return treeMaker.MethodDef(ms, mb);
}

public static CoreOp.FuncOp addVarsWhenNecessary(CoreOp.FuncOp funcOp) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something obvious. But if you have something like:

func @"f" ()java.util.Map -> {
    %0 : java.util.Map<java.lang.Integer, java.lang.Integer> = new @"func<java.util.HashMap>";
    %1 : int = constant @"1";
    %2 : int = constant @"2";
    %3 : java.lang.Object = invoke %0 %1 %2 @"java.util.Map::put(java.lang.Object, java.lang.Object)java.lang.Object";
    return %0;
}

Can't we translate as:

Map $0 = new HashMap();
int $1 = 1;
int $2 = 3;    
Object $3 = $0.put($1, $2);
return $0;

Note how this translation is very 1-1 and direct, and doesn't require any adaptation step in the model. We just emit a new local variable for each op we see in the body -- and then refer to operands via their corresponding (locally declared) variable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(of course we'd only do the above translation for ops that have a non-void result type)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can follow up on this improved translation at a later point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see. The current approach is inserting vars only when required. You're saying we can insert Vars for all operation (which is a simpler approach) ?

// using cc only is not possible
// because at first opr --> varOpRes
// at the first usage we would have to opr --> varLoad
// meaning we would have to back up the mapping, update it, then restore it before transforming the next op

Map<Value, CoreOp.VarOp> valueToVar = new HashMap<>();
AtomicInteger varCounter = new AtomicInteger();

return CoreOp.func(funcOp.funcName(), funcOp.body().bodyType()).body(block -> {
var newParams = block.parameters();
var oldParams = funcOp.parameters();
for (int i = 0; i < newParams.size(); i++) {
Op.Result var = block.op(CoreOp.var("_$" + varCounter.getAndIncrement(), newParams.get(i)));
valueToVar.put(oldParams.get(i), ((CoreOp.VarOp) var.op()));
}

block.transformBody(funcOp.body(), java.util.List.of(), (Block.Builder b, Op op) -> {
var cc = b.context();
for (Value operand : op.operands()) {
if (valueToVar.containsKey(operand)) {
var varLoadRes = b.op(CoreOp.varLoad(valueToVar.get(operand).result()));
cc.mapValue(operand, varLoadRes);
}
}
var opr = b.op(op);
var M_BLOCK_BUILDER_OP = MethodRef.method(Block.Builder.class, "op", Op.Result.class, Op.class);
var M_BLOCK_BUILDER_PARAM = MethodRef.method(Block.Builder.class, "parameter", Block.Parameter.class, TypeElement.class);
// we introduce VarOp to hold an opr that's used multiple times
// or to mark that an InvokeOp must be mapped to a Statement
// specifically call to Bloc.Builder#op, we want this call to map to a statement so that it get added
// to the opMethod body immediately to ensure correct order of operations
var isBlockOpInvocation = op instanceof CoreOp.InvokeOp invokeOp && M_BLOCK_BUILDER_OP.equals(invokeOp.invokeDescriptor());
var isBlockParamInvocation = op instanceof CoreOp.InvokeOp invokeOp && M_BLOCK_BUILDER_PARAM.equals(invokeOp.invokeDescriptor());
if (!(op instanceof CoreOp.VarOp) && (op.result().uses().size() > 1 || isBlockOpInvocation || isBlockParamInvocation)) {
var varOpRes = b.op(CoreOp.var("_$" + varCounter.getAndIncrement(), opr));
valueToVar.put(op.result(), ((CoreOp.VarOp) varOpRes.op()));
}
return b;
});
});
}

// TODO see if we can use LET AST node
// TODO add vars in OpBuilder (later)


// TODO explore builderOp --> java code (as string)
}
6 changes: 6 additions & 0 deletions src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/CodeReflectionSymbols.java
View file
Original file line number Diff line number Diff line change
@@ -48,6 +48,8 @@ public class CodeReflectionSymbols {
public final MethodSymbol methodHandlesLookup;
public final Type opType;
public final Type funcOpType;
public final Type opFactoryType;
public final Type typeElementFactoryType;

CodeReflectionSymbols(Context context) {
Symtab syms = Symtab.instance(context);
@@ -77,5 +79,9 @@ public class CodeReflectionSymbols {
syms.methodHandlesType.tsym);
syms.synthesizeEmptyInterfaceIfMissing(quotedType);
syms.synthesizeEmptyInterfaceIfMissing(quotableType);
opFactoryType = syms.enterClass(jdk_incubator_code, "jdk.incubator.code.op.OpFactory");
typeElementFactoryType = syms.enterClass(jdk_incubator_code, "jdk.incubator.code.type.TypeElementFactory");


}
}
Loading