RFR: 8325495: C2: implement optimization for series of Add of unique value [v6]

[Kangcheng Xu]

On Mon, 30 Sep 2024 13:40:59 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Kangcheng Xu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 33 commits:
>> 
>>  - resolve conflicts
>>  - resolve conflicts
>>  - Arithmetic canonicalization v3 (#3)
>>    
>>    * 8340144: C1: remove unused Compilation::_max_spills
>>    
>>    Reviewed-by: thartmann, shade
>>    
>>    * 8340176: Replace usage of -noclassgc with -Xnoclassgc in test/jdk/java/lang/management/MemoryMXBean/LowMemoryTest2.java
>>    
>>    Reviewed-by: kevinw, lmesnik
>>    
>>    * 8339793: Fix incorrect APX feature enabling with -XX:-UseAPX
>>    
>>    Reviewed-by: kvn, thartmann, sviswanathan
>>    
>>    * 8340184: Bug in CompressedKlassPointers::is_in_encodable_range
>>    
>>    Reviewed-by: coleenp, rkennke, jsjolen
>>    
>>    * 8332442: C2: refactor Mod cases in Compile::final_graph_reshaping_main_switch()
>>    
>>    Reviewed-by: roland, chagedorn, jkarthikeyan
>>    
>>    * 8340119: Remove oopDesc::size_might_change()
>>    
>>    Reviewed-by: stefank, iwalulya
>>    
>>    * 8340009: Improve the output from assert_different_registers
>>    
>>    Reviewed-by: aboldtch, dholmes, shade, mli
>>    
>>    * 8340273: Remove CounterHalfLifeTime
>>    
>>    Reviewed-by: chagedorn, dholmes
>>    
>>    * 8338566: Lazy creation of exception instances is not thread safe
>>    
>>    Reviewed-by: shade, kvn, dlong
>>    
>>    * 8339648: ZGC: Division by zero in rule_major_allocation_rate
>>    
>>    Reviewed-by: aboldtch, lucy, tschatzl
>>    
>>    * 8329816: Add SLEEF version 3.6.1
>>    
>>    Reviewed-by: erikj, mli, luhenry
>>    
>>    * 8315273: (fs) Path.toRealPath(LinkOption.NOFOLLOW_LINKS) fails when "../../" follows a link (win)
>>    
>>    Reviewed-by: djelinski
>>    
>>    * 8339574: Behavior of File.is{Directory,File,Hidden} is not documented with respect to symlinks
>>    
>>    Reviewed-by: djelinski, alanb
>>    
>>    * 8340200: Misspelled constant `AttributesProcessingOption.DROP_UNSTABLE_ATRIBUTES`
>>    
>>    Reviewed-by: liach
>>    
>>    * 8339934: Simplify Math.scalb(double) method
>>    
>>    Reviewed-by: darcy
>>    
>>    * 8339790: Support Intel APX setzucc instruction
>>    
>>    Reviewed-by: sviswanathan, jkarthikeyan, kvn
>>    
>>    * 8340323: Test jdk/classfile/OptionsTest.java fails after JDK-8340200
>>    
>>    Reviewed-by: alanb
>>    
>>    * 8338686: App classpath mismatch if a jar from the Class-Path attribute is on the classpath
>>    
>>    Reviewed-by: dholmes, iklam
>>    
>>    * 8337563: NMT: rename MEMFLAGS to MemTag
>>    
>>   ...
>
> src/hotspot/share/opto/addnode.cpp line 409:
> 
>> 407: // Convert a + a + ... + a into a*n
>> 408: Node* AddNode::convert_serial_additions(PhaseGVN* phase, bool can_reshape, BasicType bt) {
>> 409:   if (is_optimized_multiplication()) {
> 
> What would happen if that `if` is removed? Would matching in the rest of the method accept something that's not a valid pattern?

It wouldn't accept invalid patterns. It still makes (semantically) correct transformation but there is a risk it can't progress. The `if` here is to safeguard repeatedly adding terms of an already optimized mul node. Otherwise there would be an eventual timeout/live node limit situation. (E.g., `3 * a` => `(a << 2) + a` => `3 * a` => ...)

> src/hotspot/share/opto/addnode.cpp line 562:
> 
>> 560:   // swap LShiftNode to lhs for easier matching
>> 561:   if (!lhs->is_LShift()) {
>> 562:     Node* tmp = lhs;
> 
> You can use swap() here.

Good point. That is more idiomatic c++.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20754#discussion_r1781309529
PR Review Comment: https://git.openjdk.org/jdk/pull/20754#discussion_r1781309628