Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 16:01:05 GMT, Aleksey Shipilev wrote:
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
> A similar thing is already used in JDK:
> https://github.com/openjdk/jdk/blob/2a01c798d346656a0ee3553c0964feab75b5dfb6/src/java.base/share/classes/java/lang/invoke/Invokers.java#L622-L624
>
> Extending this for more common use allows doing things like optimizing
> `Integer.toString(int)`:
>
>
> @Stable
> static final String[] CONST_STRINGS = {"-1", "0", "1"};
>
> @IntrinsicCandidate
> public static String toString(int i) {
> if (isCompileConstant(i) && (i >= -1) && (i <= 1)) {
> return CONST_STRINGS[i + 1];
> }
> ...
>
>
> Note how this code would fold away to one of the paths, depending on whether
> the compiler knows it is a constant or not. Generated-code-wise it is a
> zero-cost thing :)
> @shipilev Thanks a lot for the detailed reviews and suggestions, I hope I
> have addressed all of them.
Sure thing, I just effectively merged my draft implementation into yours :)
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906602556
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 16:01:05 GMT, Aleksey Shipilev wrote:
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
> A similar thing is already used in JDK:
> https://github.com/openjdk/jdk/blob/2a01c798d346656a0ee3553c0964feab75b5dfb6/src/java.base/share/classes/java/lang/invoke/Invokers.java#L622-L624
>
> Extending this for more common use allows doing things like optimizing
> `Integer.toString(int)`:
>
>
> @Stable
> static final String[] CONST_STRINGS = {"-1", "0", "1"};
>
> @IntrinsicCandidate
> public static String toString(int i) {
> if (isCompileConstant(i) && (i >= -1) && (i <= 1)) {
> return CONST_STRINGS[i + 1];
> }
> ...
>
>
> Note how this code would fold away to one of the paths, depending on whether
> the compiler knows it is a constant or not. Generated-code-wise it is a
> zero-cost thing :)
@shipilev Thanks a lot for the detailed reviews and suggestions, I hope I have
addressed all of them.
@kimbarrett TIL about that builtin, updated the PR description to mention that
instead. Thanks very much.
@AlanBateman Another potential usage I mentioned in the JBS issue is that
`GlobalSession` is noncloseable, but there is no way for the accessor to know
that without doing a checkcast. Using this we can eliminate the check if the
session is statically known to be a global session without imposing additional
checks on other kinds of memory sessions.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906549618
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 15:44:40 GMT, Aleksey Shipilev wrote: >> Quan Anh Mai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add more overloads > > src/hotspot/share/classfile/vmIntrinsics.hpp line 927: > >> 925: >> \ >> 926: do_class(jdk_internal_misc_JitCompiler, >> "jdk/internal/misc/JitCompiler") >>\ >> 927: do_intrinsic(_isConstantExpressionZ, >> jdk_internal_misc_JitCompiler,isConstantExpression_name, >> bool_bool_signature, F_S) \ > > It would be cleaner to follow the current naming for existing intrinsic: > > > do_intrinsic(_isCompileConstant, java_lang_invoke_MethodHandleImpl, > isCompileConstant_name, isCompileConstant_signature, F_S) \ >do_name( isCompileConstant_name, > "isCompileConstant") \ >do_alias(isCompileConstant_signature, > object_boolean_signature) \ > > > I.e. rename `isConstantExpression` -> `isCompileConstant`. It clearly > communicates that we are not dealing with expressions as arguments, and that > we underline this is the (JIT) _compile_ constant, not just a constant > expression from JLS 15.28 "Constant Expressions". > > Maybe even replace that `MHImpl` method with the new intrinsic. Yes you are right, I have renamed it to `isCompileConstant`. > src/hotspot/share/opto/c2compiler.cpp line 727: > >> 725: case vmIntrinsics::_storeStoreFence: >> 726: case vmIntrinsics::_fullFence: >> 727: case vmIntrinsics::_isConstantExpressionZ: > > Move this closer to `vmIntrinsics::_isCompileConstant:`, if not outright > replace it? I have replaced `MHImpl::isCompileConstant` with the new one. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463617016 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463616039
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 15:52:29 GMT, Alan Bateman wrote:
> Would it be possible to list further examples where this might be used?
> Asking because I'm wondering about the usability and maintainability of
> if-then-else code.
A similar thing is already used in JDK:
https://github.com/openjdk/jdk/blob/2a01c798d346656a0ee3553c0964feab75b5dfb6/src/java.base/share/classes/java/lang/invoke/Invokers.java#L622-L624
Extending this for more common use allows doing things like optimizing
`Integer.toString(int)`:
@Stable
static final String[] CONST_STRINGS = {"-1", "0", "1"};
@IntrinsicCandidate
public static String toString(int i) {
if (isCompileConstant(i) && (i >= -1) && (i <= 1)) {
return CONST_STRINGS[i + 1];
}
...
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906379544
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 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 inspired by >> `std::is_constant_evaluated` in C++. >> >> 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: > > add more overloads Would it be possible to list further examples where this might be used? Asking because I'm wondering about the usability and maintainability of if-then-else code. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906362127
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 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 inspired by >> `std::is_constant_evaluated` in C++. >> >> 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: > > add more overloads All right, this is very close :) I now have stylistic comments: src/hotspot/share/classfile/vmIntrinsics.hpp line 912: > 910: do_intrinsic(_getAndSetInt, jdk_internal_misc_Unsafe, > getAndSetInt_name, getAndSetInt_signature, F_R) \ > 911:do_name( getAndSetInt_name, > "getAndSetInt") \ > 912:do_alias(getAndSetInt_signature, > /*"(Ljava/lang/Object;JI)I"*/ getAndAddInt_signature) \ I don't think we need to do these formatting changes in this PR. src/hotspot/share/classfile/vmIntrinsics.hpp line 927: > 925: > \ > 926: do_class(jdk_internal_misc_JitCompiler, > "jdk/internal/misc/JitCompiler") > \ > 927: do_intrinsic(_isConstantExpressionZ, > jdk_internal_misc_JitCompiler,isConstantExpression_name, bool_bool_signature, > F_S) \ It would be cleaner to follow the current naming for existing intrinsic: do_intrinsic(_isCompileConstant, java_lang_invoke_MethodHandleImpl, isCompileConstant_name, isCompileConstant_signature, F_S) \ do_name( isCompileConstant_name, "isCompileConstant") \ do_alias(isCompileConstant_signature, object_boolean_signature) \ I.e. rename `isConstantExpression` -> `isCompileConstant`. It clearly communicates that we are not dealing with expressions as arguments, and that we underline this is the (JIT) _compile_ constant, not just a constant expression from JLS 15.28 "Constant Expressions". Maybe even replace that `MHImpl` method with the new intrinsic. src/hotspot/share/opto/c2compiler.cpp line 727: > 725: case vmIntrinsics::_storeStoreFence: > 726: case vmIntrinsics::_fullFence: > 727: case vmIntrinsics::_isConstantExpressionZ: Move this closer to `vmIntrinsics::_isCompileConstant:`, if not outright replace it? src/hotspot/share/opto/library_call.hpp line 2: > 1: /* > 2: * Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights > reserved. Unnecessary update? - PR Review: https://git.openjdk.org/jdk/pull/17527#pullrequestreview-1839148507 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463490470 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463493124 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463497227 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463497518
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 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 inspired by >> `std::is_constant_evaluated` in C++. >> >> 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: > > add more overloads Not a review, just a drive-by comment. >From the description for this PR: "This is inspired by >std::is_constant_evaluated in C++." I think what is being proposed here is more like gcc's `__builtin_constant_p`. std::is_constant_evaluated is a different thing, used to detect evaluation in a manifestly constexpr context. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906337427
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 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 inspired by >> `std::is_constant_evaluated` in C++. >> >> 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: > > add more overloads I get your idea. I have added overloads for all types. They will all invoke `inlint_isCompileConstant`. Given that there are now 7 methods I think a separate class is more justified. Another issue is the duplication of `isConstantExpression(Object)`, but I think a separate issue to deduplicate it would be easier. Thanks a lot. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905836929
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
> 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 inspired by `std::is_constant_evaluated` > in C++. > > 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: add more overloads - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/9dd95393..31403d6f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17527=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17527=01-02 Stats: 346 lines in 6 files changed: 333 ins; 1 del; 12 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
