On Tue, 23 Jan 2024 11:18:43 GMT, Quan Anh Mai <qa...@openjdk.org> 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
