RFR: 8262341: Refine identical code in AddI/LNode. [v2]

[Fei Gao]

On Mon, 6 Dec 2021 13:01:46 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> AddINode::Ideal() and AddlNode::Ideal() are almost identical but the
>> same logic had to be duplicated because AddINode::Ideal() tests its
>> inputs for Op_AddI, Op_SubI etc. while AddLNode::Ideal() tests for
>> Op_AddL, Op_SubL etc. This patch refactors the code so the common
>> logic is in a single method parameterized by a BasicType argument.
>> 
>> The way I've done this before in the context of int/long counted loops
>> was to use and extra virtual method operates_on(). So:
>> 
>> n->Opcode() == Op_AddI becomes n->is_Add() && n->operates_on(T_INT)
>> 
>> Working on this change made me realize that pattern doesn't work that well:
>> 
>> - it's quite a bit more verbose and converting existing code is not as
>>   mechanical as we would like to avoid conversion errors.
>> 
>> - it breaks when a class has a subclass. For instance AddNode has
>>   OrINode and OrLNode as subclasses so testing for n->is_Add() returns
>>   true with an OrI node.
>> 
>> Instead, this change introduces new functions. For instance of
>> AddI/AddL:
>> 
>> int Op_Add(BasicType bt)
>> 
>> that returns either Op_AddI or Op_AddL depending on bt. This made
>> refactoring the AddINode::Ideal() logic straightforward. I removed all
>> use of operates_on() as well and converted existing code to the new
>> Op_XXX() functions.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - T *p to -> T* p
>  - Merge branch 'master' into JDK-8262341
>  - fix

src/hotspot/share/opto/addnode.hpp line 54:

> 52:   // We also canonicalize the Node, moving constants to the right input,
> 53:   // and flatten expressions (so that 1+x+2 becomes x+3).
> 54:   virtual Node* Ideal(PhaseGVN *phase, bool can_reshape);

Should be `PhaseGVN* phase` here?

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

PR: https://git.openjdk.java.net/jdk/pull/6607