-
Notifications
You must be signed in to change notification settings - Fork 32
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>
src/jdk.incubator.code/share/classes/jdk/incubator/code/dialect/core/CoreOp.java
Outdated
Show resolved
Hide resolved
src/jdk.incubator.code/share/classes/jdk/incubator/code/dialect/core/CoreOp.java
Outdated
Show resolved
Hide resolved
src/jdk.incubator.code/share/classes/jdk/incubator/code/dialect/core/CoreOp.java
Outdated
Show resolved
Hide resolved
| * <p> | ||
| * Such a lambda operation is one with the following constraints: | ||
| * <ol> | ||
| * <li>Zero or one captured value (assuming correspondence to the {@code this} variable). |
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 is too restrictive. For method references they may or may not capture this, but for a direct invocation there will be captures e.g., the optimization pattern in HAT is a lambda expression that makes a direct call to another method, capturing variables and passing then on as arguments to the method. So you need to test out those cases with mixture of lambda parameters and captured variables.
| if (funcOp != null) { | ||
| temp.add(funcOp); | ||
| Op.Result result = blockBuilder.op(CoreOp.funcCall( | ||
| iop.invokeDescriptor().name(), |
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.
The name of the func to call differs from the transformed func's name, so the model you create is incorrect. You need to test more robustly e.g., run through the interpreter or compare text output.
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