Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v7]
On Thu, 25 Jan 2024 14:01:59 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: > > change expr to val, add examples I share Roland's concerns w.r.t. profiling. If there's any code guarded by `isCompileConstant(value) == true`, the only way to trigger its profiling is by deoptimizing from C2-generated code. I added `MHI.isCompileConstant` intrinsic as part of a point fix for a performance problem caused by Java-level code profiling/specialization happening in `java.lang.invoke`. It guards profiling logic which is pruned completely once C2 kicks in. So, absence of profiling is not a problem there. Also, there's a constraint on implementation side: the current implementation supports only parse-time folding. If a value turns into a constant later (either during parsing after the call is encountered or during post-parsing phase), it won't have any effect. So, as it is now (both on API and implementation sides) it's hard to correctly use `isCompileConstant` for more general cases. It would be helpful to see more examples illustrating possible usage scenarios. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1914052884
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Thu, 25 Jan 2024 14:48:16 GMT, Maurizio Cimadamore
wrote:
> I don't 100% buy the `MethodHandleImpl` analogy. In that case the check is
> not simply used to save a branch, but to spare spinning of a completely new
> lambda form.
Doing this to save a few intructions would not likely to worth the hassle
outside the _really performance critical paths_, but even then it might be
useful for hot JDK code. On larger examples, you can avoid memory accesses,
allocations, etc. by coding up the constant-foldable path that you know
compiler would not be able to extract when propagating constants through the
generic code. For example, giving quantitative substance to my previous example:
diff --git a/src/java.base/share/classes/java/lang/Integer.java
b/src/java.base/share/classes/java/lang/Integer.java
index 1c5b3c414ba..d50748c369e 100644
--- a/src/java.base/share/classes/java/lang/Integer.java
+++ b/src/java.base/share/classes/java/lang/Integer.java
@@ -28,4 +28,5 @@
import jdk.internal.misc.CDS;
import jdk.internal.misc.VM;
+import jdk.internal.vm.ConstantSupport;
import jdk.internal.vm.annotation.ForceInline;
import jdk.internal.vm.annotation.IntrinsicCandidate;
@@ -416,4 +417,7 @@ private static void formatUnsignedIntUTF16(int val, int
shift, byte[] buf, int l
}
+@Stable
+static final String[] TO_STRINGS = { "-1", "0", "1" };
+
/**
* Returns a {@code String} object representing the
@@ -428,4 +432,8 @@ private static void formatUnsignedIntUTF16(int val, int
shift, byte[] buf, int l
@IntrinsicCandidate
public static String toString(int i) {
+if (ConstantSupport.isCompileConstant(i) &&
+(i >= -1) && (i <= 1)) {
+return TO_STRINGS[i + 1];
+}
int size = stringSize(i);
if (COMPACT_STRINGS) {
diff --git a/test/micro/org/openjdk/bench/java/lang/Integers.java
b/test/micro/org/openjdk/bench/java/lang/Integers.java
index 43ceb5d18d2..28248593a73 100644
--- a/test/micro/org/openjdk/bench/java/lang/Integers.java
+++ b/test/micro/org/openjdk/bench/java/lang/Integers.java
@@ -91,4 +91,18 @@ public void decode(Blackhole bh) {
}
+@Benchmark
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+public String toStringConstYay() {
+return Integer.toString(0);
+}
+
+int v = 0;
+
+@Benchmark
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+public String toStringConstNope() {
+return Integer.toString(v);
+}
+
/** Performs toString on small values, just a couple of digits. */
@Benchmark
Benchmark (size) Mode Cnt Score
Error Units
Integers.toStringConstNope500 avgt 15 3,599 ?
0,034 ns/op
Integers.toStringConstNope:gc.alloc.rate.norm 500 avgt 15 48,000 ?
0,001B/op
Integers.toStringConstNope:gc.time500 avgt 15223,000
ms
Integers.toStringConstYay 500 avgt 15 0,568 ?
0,046 ns/op
Integers.toStringConstYay:gc.alloc.rate.norm 500 avgt 15 ? 10??
B/op
Think about it as simplifying/avoiding the need for full compiler intrinsics. I
could, in principle, do this by intrinsifying `Integer.toString` completely,
check the same `isCon`, and then either construct the access to some String
constant, or arrange the call to actual toString slow path. That would not be
as simple as doing the similar thing in plain Java, with just a little of
compiler support in form of `ConstantSupport`.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1910449450
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Thu, 25 Jan 2024 12:52:21 GMT, Maurizio Cimadamore
wrote:
>>> > Naive question: the right way to use this would be almost invariably be
>>> > like this:
>>> > ```
>>> > if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
>>> > // fast-path
>>> > }
>>> > // slow path
>>> > ```
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> > Right?
>>>
>>> Yes, I think so.
>>
>> But then whatever is in the fast path and `fooHasCertainStaticProperties`
>> are never profiled because never executed by the interpreter or c1. So
>> `fooHasCertainStaticProperties` will likely not be inlined and c2 will do a
>> poor (or rather not as good as you'd like) job of compiling whatever is in
>> the fast path.
>
>> > > Naive question: the right way to use this would be almost invariably be
>> > > like this:
>> > > ```
>> > > if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
>> > > // fast-path
>> > > }
>> > > // slow path
>> > > ```
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > Right?
>> >
>> >
>> > Yes, I think so.
>>
>> But then whatever is in the fast path and `fooHasCertainStaticProperties`
>> are never profiled because never executed by the interpreter or c1. So
>> `fooHasCertainStaticProperties` will likely not be inlined and c2 will do a
>> poor (or rather not as good as you'd like) job of compiling whatever is in
>> the fast path.
>
> I suppose perhaps it is implied that `fooHasCertainStaticProperties` should
> have `@ForceInline` ? But yes, there seems to be several assumptions in how
> this logic is supposed to be used, and at the moment, it seems to me more of
> a footgun than something actually useful (but I admit my ignorance on the
> subject).
> @mcimadamore Yes this is hard to use apart from the simple cases. Considering
> we have already used this technique in the `MethodHandle` implementation, I
> think there are valid use cases.
I don't 100% buy the `MethodHandleImpl` analogy. In that case the check is not
simply used to save a branch, but to spare spinning of a completely new lambda
form. That is a very heavy operation. What I'm trying to say is that I'm not
too sure how robust of a mechanism this is in the context of
micro(nano?)-optimizations (such as the one you are considering).
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1910359911
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v7]
> 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: change expr to val, add examples - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/b4445e2e..84f9f7eb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17527=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17527=05-06 Stats: 51 lines in 1 file changed: 28 ins; 4 del; 19 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
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 [v6]
On Thu, 25 Jan 2024 12:52:21 GMT, Maurizio Cimadamore
wrote:
>>> > Naive question: the right way to use this would be almost invariably be
>>> > like this:
>>> > ```
>>> > if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
>>> > // fast-path
>>> > }
>>> > // slow path
>>> > ```
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> > Right?
>>>
>>> Yes, I think so.
>>
>> But then whatever is in the fast path and `fooHasCertainStaticProperties`
>> are never profiled because never executed by the interpreter or c1. So
>> `fooHasCertainStaticProperties` will likely not be inlined and c2 will do a
>> poor (or rather not as good as you'd like) job of compiling whatever is in
>> the fast path.
>
>> > > Naive question: the right way to use this would be almost invariably be
>> > > like this:
>> > > ```
>> > > if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
>> > > // fast-path
>> > > }
>> > > // slow path
>> > > ```
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > Right?
>> >
>> >
>> > Yes, I think so.
>>
>> But then whatever is in the fast path and `fooHasCertainStaticProperties`
>> are never profiled because never executed by the interpreter or c1. So
>> `fooHasCertainStaticProperties` will likely not be inlined and c2 will do a
>> poor (or rather not as good as you'd like) job of compiling whatever is in
>> the fast path.
>
> I suppose perhaps it is implied that `fooHasCertainStaticProperties` should
> have `@ForceInline` ? But yes, there seems to be several assumptions in how
> this logic is supposed to be used, and at the moment, it seems to me more of
> a footgun than something actually useful (but I admit my ignorance on the
> subject).
@mcimadamore Yes this is hard to use apart from the simple cases. Considering
we have already used this technique in the `MethodHandle` implementation, I
think there are valid use cases.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1910271891
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Thu, 25 Jan 2024 07:41:27 GMT, Roland Westrelin wrote:
> > > Naive question: the right way to use this would be almost invariably be
> > > like this:
> > > ```
> > > if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
> > > // fast-path
> > > }
> > > // slow path
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > Right?
> >
> >
> > Yes, I think so.
>
> But then whatever is in the fast path and `fooHasCertainStaticProperties` are
> never profiled because never executed by the interpreter or c1. So
> `fooHasCertainStaticProperties` will likely not be inlined and c2 will do a
> poor (or rather not as good as you'd like) job of compiling whatever is in
> the fast path.
I suppose perhaps it is implied that `fooHasCertainStaticProperties` should
have `@ForceInline` ? But yes, there seems to be several assumptions in how
this logic is supposed to be used, and at the moment, it seems to me more of a
footgun than something actually useful (but I admit my ignorance on the
subject).
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1910140980
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Wed, 24 Jan 2024 18:56:15 GMT, Aleksey Shipilev wrote:
> > Naive question: the right way to use this would be almost invariably be
> > like this:
> > ```
> > if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
> > // fast-path
> > }
> > // slow path
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Right?
>
> Yes, I think so.
But then whatever is in the fast path and `fooHasCertainStaticProperties` are
never profiled because never executed by the interpreter or c1. So
`fooHasCertainStaticProperties` will likely not be inlined and c2 will do a
poor (or rather not as good as you'd like) job of compiling whatever is in the
fast path.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1909531426
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 [v6]
On Wed, 24 Jan 2024 18:56:15 GMT, Aleksey Shipilev wrote:
>>> Naive question: the right way to use this would be almost invariably be
>>> like this:
>>>
>>> ```
>>> if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
>>> // fast-path
>>> }
>>> // slow path
>>> ```
>>>
>>> Right? Then the expectation is that during interpreter and C1,
>>> `isCompileConstant` always returns false, so we just never take the fast
>>> path (but we probably still pay for the branch, right?). And, when we get
>>> to C2 and this method is inlined, at this point we know that either `foo`
>>> is constant or not. If it is constant we can check other conditions on foo
>>> (which presumably is cheap because `foo` is constant) and maybe take the
>>> fast-path. In both cases, there's no branch in the generated code because
>>> we know "statically" when inlining if `foo` has the right shape or not.
>>> Correct?
>>
>> P.S. if this is correct, please consider adding something along those lines
>> in the javadoc of `isCompileConstant`; as it stands it is a bit obscure to
>> understand how this thing might be used, and what are the common pitfalls to
>> avoid when using it.
>
>> Naive question: the right way to use this would be almost invariably be like
>> this:
>>
>> ```
>> if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
>> // fast-path
>> }
>> // slow path
>> ```
>>
>> Right?
>
> Yes, I think so.
>
>> Then the expectation is that during interpreter and C1, `isCompileConstant`
>> always returns false, so we just never take the fast path (but we probably
>> still pay for the branch, right?).
>
> Yes, I think so. For C1, we would still prune the "dead" path, because C1 is
> able to know that `if (false)` is never taken. We do pay with the branch and
> the method call in interpreter. (There are ways to special-case these
> intrinsics for interpreter too, if we choose to care.)
>
>> And, when we get to C2 and this method is inlined, at this point we know
>> that either `foo` is constant or not. If it is constant we can check other
>> conditions on foo (which presumably is cheap because `foo` is constant) and
>> maybe take the fast-path. In both cases, there's no branch in the generated
>> code because we know "statically" when inlining if `foo` has the right shape
>> or not. Correct?
>
> Yes. I think the major use would be using `constexpr`-like code on "const"
> path, so that the entire code constant-folds completely, _or_ just compiles
> to branch-less "generic" version. In [my
> experiments](https://github.com/openjdk/jdk/pull/17527#issuecomment-1906379544)
> with `Integer.toString` that certainly happens. But that is not a
> requirement, and we could probably still reap some benefits from partial
> constant folds; but at that point we would need to prove that a "partially
> const" path is better than generic "non-const" path under the same conditions.
>
> I agree it would be convenient to put some examples in javadoc. @merykitty, I
> can help you with that, if you want.
@shipilev I can come up with 2 examples that are pretty generic:
void checkIndex(int index, int length) {
boolean indexPositive = index >= 0;
if (ConstantSupport.isCompileConstant(indexPositive) && indexPositive) {
if (index >= length) {
throw;
}
return;
}
if (length < 0 || Integer.compareUnsigned(index, length) >= 0) {
throw;
}
}
bool equals(Point p1, Point p2) {
idEqual = p1 == p2;
if (ConstantSupport.isCompileConstant(idEqual) && idEqual) {
return true;
}
return p1.x == p2.x && p1.y == p2.y;
}
@mcimadamore Yes I believe your expectations are correct. Pitfalls may vary
case-by-case, but I just realised that since we do not have profile information
in the fast path, the compiler may be less willingly to inline the callees
here. While it has not been an issue, a solution I can think of is to have
something like `ConstantSupport::evaluate` in which the compiler will try to
inline infinitely expecting constant-folding similar to how a `constexpr`
variable behaves in C++ (and maybe bail-out compilation if the final result is
not a constant, too).
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1909272480
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 [v6]
On Wed, 24 Jan 2024 18:51:27 GMT, Maurizio Cimadamore
wrote:
> Naive question: the right way to use this would be almost invariably be like
> this:
>
> ```
> if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
> // fast-path
> }
> // slow path
> ```
>
> Right?
Yes, I think so.
> Then the expectation is that during interpreter and C1, `isCompileConstant`
> always returns false, so we just never take the fast path (but we probably
> still pay for the branch, right?).
Yes, I think so. For C1, we would still prune the "dead" path, because C1 is
able to know that `if (false)` is never taken. We do pay with the branch and
the method call in interpreter. (There are ways to special-case these
intrinsics for interpreter too, if we choose to care.)
> And, when we get to C2 and this method is inlined, at this point we know that
> either `foo` is constant or not. If it is constant we can check other
> conditions on foo (which presumably is cheap because `foo` is constant) and
> maybe take the fast-path. In both cases, there's no branch in the generated
> code because we know "statically" when inlining if `foo` has the right shape
> or not. Correct?
Yes. I think the major use would be using `constexpr`-like code on "const"
path, so that the entire "const" branch constant-folds completely. In [my
experiments](https://github.com/openjdk/jdk/pull/17527#issuecomment-1906379544)
with `Integer.toString` that certainly happens. But that is not a requirement,
and we could probably still reap some benefits from partial constant folds; but
at that point we would need to prove that a "partially const" path is better
than generic "non-const" path under the same conditions.
I agree it would be convenient to put some examples in javadoc.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1908736651
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Wed, 24 Jan 2024 18:48:03 GMT, Maurizio Cimadamore
wrote:
> Naive question: the right way to use this would be almost invariably be like
> this:
>
> ```
> if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
> // fast-path
> }
> // slow path
> ```
>
> Right? Then the expectation is that during interpreter and C1,
> `isCompileConstant` always returns false, so we just never take the fast path
> (but we probably still pay for the branch, right?). And, when we get to C2
> and this method is inlined, at this point we know that either `foo` is
> constant or not. If it is constant we can check other conditions on foo
> (which presumably is cheap because `foo` is constant) and maybe take the
> fast-path. In both cases, there's no branch in the generated code because we
> know "statically" when inlining if `foo` has the right shape or not. Correct?
P.S. if this is correct, please consider adding something along those lines in
the javadoc of `isCompileConstant`; as it stands it is a bit obscure to
understand how this thing might be used, and what are the common pitfalls to
avoid when using it.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1908729766
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Wed, 24 Jan 2024 10:33:05 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: > > address reviews src/java.base/share/classes/jdk/internal/vm/ConstantSupport.java line 32: > 30: /** > 31: * Just-in-time-compiler-related queries > 32: */ This looks like a stale comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1465397036
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 [v6]
On Wed, 24 Jan 2024 10:33:05 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:
>
> address reviews
Naive question: the right way to use this would be almost invariably be like
this:
if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
// fast-path
}
// slow path
Right? Then the expectation is that during interpreter and C1,
`isCompileConstant` always returns false, so we just never take the fast path
(but we probably still pay for the branch, right?). And, when we get to C2 and
this method is inlined, at this point we know that either `foo` is constant or
not. If it is constant we can check other conditions on foo (which presumably
is cheap because `foo` is constant) and maybe take the fast-path. In both
cases, there's no branch in the generated code because we know "statically"
when inlining if `foo` has the right shape or not. Correct?
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1908724632
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 [v6]
> 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: address reviews - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/3ecb2c66..b4445e2e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17527=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17527=04-05 Stats: 36 lines in 4 files changed: 10 ins; 0 del; 26 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
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 [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 [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 [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 [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
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 [v4]
> 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: address reviews: rename to isCompileConstant, remove duplication, revert unnecessary changes - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/31403d6f..18f7d482 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17527=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17527=02-03 Stats: 92 lines in 8 files changed: 10 ins; 13 del; 69 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
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
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 09:31:51 GMT, Quan Anh Mai wrote: > Maybe I am ignorant but doesn't the definition of an intrinsics contain the > signature of the method as well? The definitions in `vmIntrinsics`, sure, they require full signature for `@IntrinsicCandidate` methods. It would yield some unfortunate duplication. But after that, we can map on the same `inline_isCompileConstant` intrinsic that just asks `arg(0)->is_Con()`, and it would not care about the type of the constant. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905667006
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:16:07 GMT, Aleksey Shipilev 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.
>
> Nice. I had a similar thing stashed in my todo queue. Note that there is
> already `isCompileConstant` that does similar thing:
> https://github.com/openjdk/jdk/blob/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/hotspot/share/opto/library_call.cpp#L8189-L8193
> -- maybe we should just expose that more widely. I would suggest we just do
> the private `java.lang.{Integer,...}.isCompileConstant` methods and bind them
> to that intrinsic.
> @shipilev Thanks a lot for your suggestions. Yes I can just use
> `inline_isCompileConstant` instead.
>
> Regarding the place of the method, I'm not really sure as putting in
> `java.lang.Long` seems out-of-place for an internal mechanism that is
> obviously not only used in `java.lang`, which will force a new entry in
> `JavaLangAccess`.
Ah yes, if you need to use it across module boundaries, putting the
private/protected method would require `JavaLangAccess`, which is burdensome. I
am just icky about introducing a whole new internal class for this. Is there
anything in current `jdk.internal.vm.*` that fits it? Maybe `misc.Unsafe` or
`misc.VM`?
> Finally, I think accepting a `long` would be enough (maybe `double`, too?)
> since `int`, `boolean` etc can be converted losslessly to `long`.
Right, that would work for primitives, since we could probably rely on
conversion for constants to be folded. But I also see the value for asking
`isCompileConstant(Object)`, which is not easily convertible. So I would just
do the overloads for all primitives and `Object`. The C2 intrinsic would not
care about the `arg(0)` type, it would reply `isCon` on those constants just
the same.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905655407
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:16:07 GMT, Aleksey Shipilev wrote:
> I would suggest we just do the private
> `java.lang.{Integer,...}.isCompileConstant` methods and bind them to that
> intrinsic.
Maybe I am ignorant but doesn't the definition of an intrinsics contain the
signature of the method as well?
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905648723
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:16:07 GMT, Aleksey Shipilev 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.
>
> Nice. I had a similar thing stashed in my todo queue. Note that there is
> already `isCompileConstant` that does similar thing:
> https://github.com/openjdk/jdk/blob/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/hotspot/share/opto/library_call.cpp#L8189-L8193
> -- maybe we should just expose that more widely. I would suggest we just do
> the private `java.lang.{Integer,...}.isCompileConstant` methods and bind them
> to that intrinsic.
@shipilev Thanks a lot for your suggestions. Yes I can just use
`inline_isCompileConstant` instead.
Regarding the place of the method, I'm not really sure as putting in
`java.lang.Long` seems out-of-place for an internal mechanism that is obviously
not only used in `java.lang`, which will force a new entry in `JavaLangAccess`.
Finally, I think accepting a `long` would be enough (maybe `double`, too?)
since `int`, `boolean` etc can be converted losslessly to `long`.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905641141
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v2]
> 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: use inline_isCompileConstant - Changes: - all: https://git.openjdk.org/jdk/pull/17527/files - new: https://git.openjdk.org/jdk/pull/17527/files/4d0fc3dd..9dd95393 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17527=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17527=00-01 Stats: 13 lines in 3 files changed: 0 ins; 9 del; 4 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
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:10:54 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.
Nice. I had a similar thing stashed in my todo queue. Note that there is
already `isCompileConstant` that does similar thing:
https://github.com/openjdk/jdk/blob/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/hotspot/share/opto/library_call.cpp#L8189-L8193
-- maybe we should just expose that more widely. I would suggest we just do
the private `java.lang.{Integer,...}.isCompileConstant` methods and bind them
to that intrinsic.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905504206
RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
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. - Commit messages: - bug number - add isConstantExpression Changes: https://git.openjdk.org/jdk/pull/17527/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17527=00 Issue: https://bugs.openjdk.org/browse/JDK-8324433 Stats: 162 lines in 6 files changed: 158 ins; 0 del; 4 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
