Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Thu, 25 Jan 2024 05:06:12 GMT, David Holmes wrote: >> I agree. All values are produced by evaluating expressions. In this case we >> want to query whether a value produced by the compiler evaluating its >> expression is a constant value (inputs to the expression are constants and >> the expression had no material side-effects). Meaning if the method returns >> true then we could use that knowledge in subsequent expressions that may >> also produce constants or some specific behavior. > >> the method compilation has the expression in its original form > > So the JIT analyses the bytecode used to place the result on the call stack, > before the call, and from that determines if the expression were a constant? > This kind of self-analysis is not something I was aware of. I see, changed `expr` to `val`. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1466418470
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Wed, 24 Jan 2024 19:37:40 GMT, Paul Sandoz wrote: >> It is still weird to talk about expressions at this level. We really check >> if the value is constant, like the method name suggests now. Yes, this >> implicitly tests that the expression that produced that value is fully >> constant-folded. But that's a detail that we do not need to capture here. >> Let's rename `expr` -> `val`, and tighten up the javadoc for the method to >> mention we only test the constness of the final value. > > I agree. All values are produced by evaluating expressions. In this case we > want to query whether a value produced by the compiler evaluating its > expression is a constant value (inputs to the expression are constants and > the expression had no material side-effects). Meaning if the method returns > true then we could use that knowledge in subsequent expressions that may also > produce constants or some specific behavior. > the method compilation has the expression in its original form So the JIT analyses the bytecode used to place the result on the call stack, before the call, and from that determines if the expression were a constant? This kind of self-analysis is not something I was aware of. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1465846860
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Wed, 24 Jan 2024 18:48:34 GMT, Aleksey Shipilev wrote: >> @dholmes-ora Indeed it's a compiler magic, albeit not really weird. While >> the method execution only receives the evaluated value of `expr`, the method >> compilation has the expression in its original form. As a result, it can >> determine the result based on this information. > > It is still weird to talk about expressions at this level. We really check if > the value is constant, like the method name suggests now. Yes, this > implicitly tests that the expression that produced that value is fully > constant-folded. But that's a detail that we do not need to capture here. > Let's rename `expr` -> `val`, and tighten up the javadoc for the method to > mention we only test the constness of the final value. I agree. All values are produced by evaluating expressions. In this case we want to query whether a value produced by the compiler evaluating its expression is a constant value (inputs to the expression are constants and the expression had no material side-effects). Meaning if the method returns true then we could use that knowledge in subsequent expressions that may also produce constants or some specific behavior. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1465449454
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Wed, 24 Jan 2024 07:15:12 GMT, Quan Anh Mai wrote: >> This seems really weird to me for Java code. The method doesn't get the >> original "expression" it only gets the value of that expression after it has >> been evaluated. Is there some kind of weird "magic" happening here? > > @dholmes-ora Indeed it's a compiler magic, albeit not really weird. While the > method execution only receives the evaluated value of `expr`, the method > compilation has the expression in its original form. As a result, it can > determine the result based on this information. It is still weird to talk about expressions at this level. We really check if the value is constant, like the method name suggests now. Yes, this implicitly tests that the expression that produced that value is fully constant-folded. But that's a detail that we do not need to capture here. Let's rename `expr` -> `val`, and tighten up the javadoc for the method to mention we only test the constness of the final value. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1465401456
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Wed, 24 Jan 2024 09:03:43 GMT, Aleksey Shipilev wrote: >> That sounds like a better name for the class, although I think >> `jdk.internal.misc` is more suitable than `jdk.internal.vm`. Do you have any >> preference? Thanks. > > +1 to `ConstantSupport`. I think `jdk.internal.vm` is a proper place for it. > There is adjacent `jdk.internal.vm.vector.VectorSupport`, and whole > `jdk.internal.vm.annotations` package is there too. > > `jdk.internal.misc` sounds like a place for utility classes. `Unsafe` is a > historical exception, I think. I see, my main premise is that it is somewhat similar to `Unsafe` which turns out to be an exception :) Thanks a lot for your suggestions, I have updated the PR, also added `@Hidden` back. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464707689
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 22:49:49 GMT, Quan Anh Mai wrote:
>> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 32:
>>
>>> 30: * Just-in-time-compiler-related queries
>>> 31: */
>>> 32: public class JitCompiler {
>>
>> An alternative name and location is `jdk.internal.vm.ConstantSupport` with
>> initial class doc:
>>
>> Defines methods to test if a value has been evaluated to a compile-time
>> constant value by the HotSpot VM.
>
> That sounds like a better name for the class, although I think
> `jdk.internal.misc` is more suitable than `jdk.internal.vm`. Do you have any
> preference? Thanks.
+1 to `ConstantSupport`. I think `jdk.internal.vm` is a proper place for it.
There is adjacent `jdk.internal.vm.vector.VectorSupport`, and whole
`jdk.internal.vm.annotations` package is there too.
`jdk.internal.misc` sounds like a place for utility classes. `Unsafe` is a
historical exception, I think.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464551793
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 22:41:44 GMT, Quan Anh Mai wrote: >> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 119: >> >>> 117: * @see #isCompileConstant(boolean) >>> 118: */ >>> 119: @IntrinsicCandidate >> >> Note how the Java entry for MH intrinsic we have replaced had `@Hidden`. >> These methods should have `@Hidden` too then? Probably applies to other >> entries too. > > I don't understand why this needs to be `@Hidden`, the javadoc says that a > function annotated with `@Hidden` is omitted from the stacktraces. This > function does not call anything so what is the point of hiding it? I suspect there is a code that counts stack traces somewhere that relies on it in MH parts. There is no harm for doing `@Hidden` here, I think. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464541674
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Wed, 24 Jan 2024 06:27:20 GMT, David Holmes wrote: >> I think of this as an expression that is always evaluated to the same value. >> The value itself is not interesting, it is the set of values that this >> expression can take that we are talking about. > > This seems really weird to me for Java code. The method doesn't get the > original "expression" it only gets the value of that expression after it has > been evaluated. Is there some kind of weird "magic" happening here? @dholmes-ora Indeed it's a compiler magic, albeit not really weird. While the method execution only receives the evaluated value of `expr`, the method compilation has the expression in its original form. As a result, it can determine the result based on this information. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464415357
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 22:46:20 GMT, Quan Anh Mai wrote:
>> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 56:
>>
>>> 54: */
>>> 55: @IntrinsicCandidate
>>> 56: public static boolean isCompileConstant(boolean expr) {
>>
>> Here and in other places: probably not `expr`, but just `val` or something?
>
> I think of this as an expression that is always evaluated to the same value.
> The value itself is not interesting, it is the set of values that this
> expression can take that we are talking about.
This seems really weird to me for Java code. The method doesn't get the
original "expression" it only gets the value of that expression after it has
been evaluated. Is there some kind of weird "magic" happening here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464361310
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 20:01:45 GMT, Paul Sandoz wrote:
>> Quan Anh Mai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> ident
>
> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 32:
>
>> 30: * Just-in-time-compiler-related queries
>> 31: */
>> 32: public class JitCompiler {
>
> An alternative name and location is `jdk.internal.vm.ConstantSupport` with
> initial class doc:
>
> Defines methods to test if a value has been evaluated to a compile-time
> constant value by the HotSpot VM.
That sounds like a better name for the class, although I think
`jdk.internal.misc` is more suitable than `jdk.internal.vm`. Do you have any
preference? Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464087772
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:42:40 GMT, Aleksey Shipilev wrote:
>> Quan Anh Mai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> ident
>
> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 56:
>
>> 54: */
>> 55: @IntrinsicCandidate
>> 56: public static boolean isCompileConstant(boolean expr) {
>
> Here and in other places: probably not `expr`, but just `val` or something?
I think of this as an expression that is always evaluated to the same value.
The value itself is not interesting, it is the set of values that this
expression can take that we are talking about.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464085126
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:40:52 GMT, Aleksey Shipilev wrote: >> Quan Anh Mai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> ident > > src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 119: > >> 117: * @see #isCompileConstant(boolean) >> 118: */ >> 119: @IntrinsicCandidate > > Note how the Java entry for MH intrinsic we have replaced had `@Hidden`. > These methods should have `@Hidden` too then? Probably applies to other > entries too. I don't understand why this needs to be `@Hidden`, the javadoc says that a function annotated with `@Hidden` is omitted from the stacktraces. This function does not call anything so what is the point of hiding it? - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464081953
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:21:47 GMT, Quan Anh Mai wrote:
>> Hi,
>>
>> This patch introduces `JitCompiler::isConstantExpression` which can be used
>> to statically determine whether an expression has been constant-folded by
>> the Jit compiler, leading to more constant-folding opportunities. For
>> example, it can be used in `MemorySessionImpl::checkValidStateRaw` to
>> eliminate the lifetime check on global sessions without imposing additional
>> branches on other non-global sessions. This is similar to
>> `__builtin_constant_p` in GCC and clang.
>>
>> Please kindly give your opinion as well as your reviews, thanks very much.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> ident
src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 32:
> 30: * Just-in-time-compiler-related queries
> 31: */
> 32: public class JitCompiler {
An alternative name and location is `jdk.internal.vm.ConstantSupport` with
initial class doc:
Defines methods to test if a value has been evaluated to a compile-time
constant value by the HotSpot VM.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463926393
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:21:47 GMT, Quan Anh Mai wrote:
>> Hi,
>>
>> This patch introduces `JitCompiler::isConstantExpression` which can be used
>> to statically determine whether an expression has been constant-folded by
>> the Jit compiler, leading to more constant-folding opportunities. For
>> example, it can be used in `MemorySessionImpl::checkValidStateRaw` to
>> eliminate the lifetime check on global sessions without imposing additional
>> branches on other non-global sessions. This is similar to
>> `__builtin_constant_p` in GCC and clang.
>>
>> Please kindly give your opinion as well as your reviews, thanks very much.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> ident
A few more stylistic comments :)
Still thinking the better home for these might be just
`jdk.internal.misc.VM`... But I would not insist, if others are happy.
src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 56:
> 54: */
> 55: @IntrinsicCandidate
> 56: public static boolean isCompileConstant(boolean expr) {
Here and in other places: probably not `expr`, but just `val` or something?
src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 119:
> 117: * @see #isCompileConstant(boolean)
> 118: */
> 119: @IntrinsicCandidate
Note how the Java entry for MH intrinsic we have replaced had `@Hidden`. These
methods should have `@Hidden` too then? Probably applies to other entries too.
-
PR Review: https://git.openjdk.org/jdk/pull/17527#pullrequestreview-1839475907
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463705907
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463703771
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
> Hi, > > This patch introduces `JitCompiler::isConstantExpression` which can be used > to statically determine whether an expression has been constant-folded by the > Jit compiler, leading to more constant-folding opportunities. For example, it > can be used in `MemorySessionImpl::checkValidStateRaw` to eliminate the > lifetime check on global sessions without imposing additional branches on > other non-global sessions. This is similar to `__builtin_constant_p` in GCC > and clang. > > Please kindly give your opinion as well as your reviews, thanks very much. Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision: ident - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/18f7d482..3ecb2c66 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17527=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17527=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17527/head:pull/17527 PR: https://git.openjdk.org/jdk/pull/17527
