On Fri, 14 May 2021 07:27:08 GMT, Claes Redestad <redes...@openjdk.org> wrote:
>> Jorn Vernee has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - - Formatting >> - Reduce benchmark cases >> - Remove NamedFunction::intrinsicName >> - All changes prior to review > > Overall this looks great! > > I have a number of nits and a request to try and remodel so that > `NamedFunction` isn't responsible for holding the arbitrary intrinsic data > that you want to add here. Thanks for the review @cl4es I've addressed your review comments. I've reduced the number of cases in the benchmark to 5, 10, and 25, which is the most interesting area to look at, and also removed the offset cases for the non-constant input benchmarks (proving the offset is constant folded only needs to be done once I think). WRT `NamedFunction::intrinsicName`, I think you're right that we can also just delegate to `IntrinsicMethodHandle::intrinsicName`, and have it indicate the intrinsic. I've implemented that change. As a result, some `NamedFunction` constructor call sites no longer attach the intrinsic name to the `NamedFunction` itself, but re-wrap the `resolvedHandle` they use in an `IntrinsicMethodHandle` with the right intrinsic name. This leads to a nice code simplification from being able to remove all the `NamedFunction` constructor overloads. java/lang/invoke tests are still green, but I'll re-run Tier 1-3 as well to make sure that the difference in `resolvedHandle` being used for some `NamedFunctions` doesn't cause any other problems. ------------- PR: https://git.openjdk.java.net/jdk/pull/3401
