On Sun, 23 Jun 2024 08:47:25 GMT, Shaojin Wen wrote:
>> After PR https://github.com/openjdk/jdk/pull/16245, C2 optimizes stores into
>> primitive arrays by combining values into larger stores.
>>
>> This PR rewrites the code of appendNull and append(boolean) methods so that
>> these two methods can be optimized by C2.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> private static final field `UNSAFE`
I'm running your benchmark
[PutCharTest.java](https://github.com/user-attachments/files/15974672/PutCharTest.txt)
with:
/oracle-work/jdk-fork2/build/linux-x64-slowdebug/jdk/bin/java --add-modules
java.base --add-exports java.base/jdk.internal.misc=ALL-UNNAMED --add-exports
java.base/jdk.internal.util=ALL-UNNAMED -XX:+TraceMergeStores -Xbatch
-XX:CompileCommand=printcompilation,PutCharTest::*
-XX:CompileCommand=compileonly,PutCharTest::put* -XX:+PrintIdeal
PutCharTest.java
Aha, I found the limitation in `MergeStores`:
https://github.com/openjdk/jdk/blob/f7862bd6b9994814c6dfd43d471122408601f288/src/hotspot/share/opto/memnode.cpp#L2982-L2986
Basically, I require the `store` to have the same data-size as the element-size
of the array.
But the `putChar` ends up as a 2-byte `StoreC`, and the array is a `byte[]`
with 1-byte elements.
I quickly removed this check:
diff --git a/src/hotspot/share/opto/memnode.cpp
b/src/hotspot/share/opto/memnode.cpp
index d0b6c59637f..78efda2db13 100644
--- a/src/hotspot/share/opto/memnode.cpp
+++ b/src/hotspot/share/opto/memnode.cpp
@@ -2980,10 +2980,10 @@ StoreNode* MergePrimitiveArrayStores::run() {
return nullptr;
}
BasicType bt = aryptr_t->elem()->array_element_basic_type();
- if (!is_java_primitive(bt) ||
- type2aelembytes(bt) != _store->memory_size()) {
-return nullptr;
- }
+ //if (!is_java_primitive(bt) ||
+ //type2aelembytes(bt) != _store->memory_size()) {
+ // return nullptr;
+ //}
// The _store must be the "last" store in a chain. If we find a use we could
merge with
// then that use or a store further down is the "last" store.
@@ -3033,13 +3033,13 @@ bool
MergePrimitiveArrayStores::is_compatible_store(const StoreNode* other_store
if (!is_java_primitive(aryptr_bt1) || !is_java_primitive(aryptr_bt2)) {
return false;
}
- int size1 = type2aelembytes(aryptr_bt1);
- int size2 = type2aelembytes(aryptr_bt2);
- if (size1 != size2 ||
- size1 != _store->memory_size() ||
- _store->memory_size() != other_store->memory_size()) {
-return false;
- }
+ //int size1 = type2aelembytes(aryptr_bt1);
+ //int size2 = type2aelembytes(aryptr_bt2);
+ //if (size1 != size2 ||
+ //size1 != _store->memory_size() ||
+ //_store->memory_size() != other_store->memory_size()) {
+ // return false;
+ //}
return true;
}
And now it optimizes:
15523 103b 4 PutCharTest::putNull (14 bytes)
[TraceMergeStores]: Replace
74 StoreC === 62 7 73 22 [[ 99 ]] @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; mismatched
unsafe Memory: @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; !jvms:
PutCharTest::putChar @ bci:14 (line 14) PutCharTest::putCharsAt @ bci:4 (line
7) PutCharTest::putNull @ bci:10 (line 23)
99 StoreC === 62 74 98 23 [[ 124 ]] @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; mismatched
unsafe Memory: @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; !jvms:
PutCharTest::putChar @ bci:14 (line 14) PutCharTest::putCharsAt @ bci:13 (line
8) PutCharTest::putNull @ bci:10 (line 23)
124 StoreC === 62 99 123 24 [[ 150 ]] @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; mismatched
unsafe Memory: @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; !jvms:
PutCharTest::putChar @ bci:14 (line 14) PutCharTest::putCharsAt @ bci:23 (line
9) PutCharTest::putNull @ bci:10 (line 23)
150 StoreC === 62 124 149 24 [[ 17 ]] @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; mismatched
unsafe Memory: @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; !jvms:
PutCharTest::putChar @ bci:14 (line 14) PutCharTest::putCharsAt @ bci:33 (line
10) PutCharTest::putNull @ bci:10 (line 23)
[TraceMergeStores]: with
155 ConL === 0 [[ 156 ]] #long:30399761348886638
156 StoreL === 62 7 73 155 [[ ]] @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4;
mismatched Memory: @byte[int:>=0]
(java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4;
I will file an RFE to enable this use-case as well. @wenshao thanks for the
standalone test, that was really helpful!
-
PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2189509983
