RFR: 8262837: handle split_USE correctly

[Vladimir Kozlov]

On Tue, 2 Mar 2021 09:26:21 GMT, kuaiwei <github.com+1981974+kuaiwei at openjdk.org> wrote:

> I look into reg_split.cpp and found a potential issue.
> Function split_USE usually creates a new spill copy node, but in some cases it just connect def to use and no new node created. But in caller side, they will assume it will create a new node. The code is like:
> ++
> maxlrg = split_USE(...)
> if (!maxlrg) {
>    return 0;
> }
> insidx++;  // Reset iterator to skip USE side split
> So if no node is created, the iterator index is updated and the next instruction will be skipped.
> 
> The change is let split_USE return the new node count, so the caller can update its index and maxlrg.

Changes requested by kvn (Reviewer).

src/hotspot/share/opto/reg_split.cpp line 230:

> 228:         }
> 229:         // insert into basic block
> 230:         insert_proj( b, bindex, spill, maxlrg++ );

Do we need `maxlrg` post-increment here with this change?

src/hotspot/share/opto/reg_split.cpp line 279:

> 277:   // Insert SpillCopy before the USE, which uses the reaching DEF as
> 278:   // its input, and defs a new live range, which is used by this node.
> 279:   insert_proj( b, bindex, spill, maxlrg++ );

`maxlrg++`  again

src/hotspot/share/opto/reg_split.cpp line 1106:

> 1104:                   const RegMask* tmp_rm = Matcher::idealreg2regmask[def_ideal];
> 1105:                   Node *spill = new MachSpillCopyNode(MachSpillCopyNode::MemToReg, def, dmask, *tmp_rm);
> 1106:                   insert_proj( b, insidx, spill, maxlrg );

I think it missed `maxlrg` increment. May be also use `insidx++` here so in the following code you need to increment only by `delta`.

src/hotspot/share/opto/reg_split.cpp line 501:

> 499:   uint                 bidx, pidx, slidx, insidx, inpidx, twoidx;
> 500:   uint                 non_phi = 1, spill_cnt = 0;
> 501:   int                  delta;

I think it should be local variable where it is used.

src/hotspot/share/opto/reg_split.cpp line 283:

> 281:   use->set_req(useidx,spill);
> 282: 
> 283:   // return generated node count

The comment is confusing. I would suggest to add comment at the header of this method to describe all returned values [-1, 0, 1].

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

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