-
Notifications
You must be signed in to change notification settings - Fork 31
ModuleOp generation from LambdaOp and FuncOp #691
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.
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
base: code-reflection
Are you sure you want to change the base?
Conversation
|
👋 Welcome back rbrchen! A progress list of the required criteria for merging this PR into |
|
@rbrchen This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
src/jdk.incubator.code/share/classes/jdk/incubator/code/dialect/core/CoreOp.java
Outdated
Show resolved
Hide resolved
…t/core/CoreOp.java Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
| while (!stack.isEmpty()) { | ||
| CoreOp.FuncOp cur = stack.removeLast(); | ||
| if (!visited.add(cur)) continue; | ||
| funcs.add(convertInvokeToFuncCallOp(cur, l)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are traversing the model and resolving methods twice, you can combine the two. If you want to keep it a separate method then pass the temporary list of the encountered invokes to reflectable methods.
|
|
||
| stack.add(f); | ||
| while (!stack.isEmpty()) { | ||
| CoreOp.FuncOp cur = stack.removeLast(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it easier to think in terms of pop/push. I see why you are doing this, to preserve the order of the call graph. Add a comment why you are working from the other end of the stack. You can accumulate into an array list (not a linked list) then do stack.addAll(temp.reversed()) (also choose a better name than temp).
Alternatively use pop and push, accumulating into a list and then pushing on the stack like so:
for (FuncOp f : invokedFuncs.reversed()) {
work.push(f);
}
| boolean methodNameExists = Arrays.stream(l.lookupClass().getMethods()) | ||
| .anyMatch(method -> method.getName().equals(lambdaName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that check is right. To do this properly we would have to check against the names of methods encountered in the call graph. In general we have another naming clash problem with overloaded methods. A simple approach is to always postfix the function name with the topologically sorted index e.g. "_"
| Consumer<Integer> lambda = (@Reflect Consumer<Integer>) (d) -> {d += a + b + c;}; | ||
| LambdaOp qop = (LambdaOp) Op.ofQuotable(lambda).get().op(); | ||
| FuncOp funcOp = qop.toFuncOp(null); | ||
| System.out.println(funcOp.toText()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not making any assertions. Use the interpreter to invoke the function and lambda with the same arguments, and assert they produce the same result.
Add support for generating a ModuleOp from either a LambdaOp or a FuncOp. The given LambdaOp will be converted into the root FuncOp, and the given FuncOp will be assigned as the root FuncOp.
For
CoreOp.ModuleOp.ofLambdaOp(), a unique name for the new root FuncOp must be passed as a parameter.Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/691/head:pull/691$ git checkout pull/691Update a local copy of the PR:
$ git checkout pull/691$ git pull https://git.openjdk.org/babylon.git pull/691/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 691View PR using the GUI difftool:
$ git pr show -t 691Using diff file
Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/691.diff
Using Webrev
Link to Webrev Comment