RFR: 8282555: Missing memory edge when spilling MoveF2I, MoveD2L etc

[Emanuel Peter]

On Tue, 29 Mar 2022 10:20:20 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>>> @jatin-bhateja Yes, the problem is that we overwrite the input-edge to the spill-definition (`src`), with the frame-pointer (`fp`). Good to hear that you agree that we need that extra edge from the spill-definition.
>>> 
>>> We do add back in the spill-definition (`src`), but only if the node permits it: [condition](https://github.com/openjdk/jdk/blob/6ed0ba2f8a2af58c45a6b7be684ef30d15af6ead/src/hotspot/share/opto/chaitin.cpp#L1727): `cisc->oper_input_base() > 1 && mach->oper_input_base() <= 1` [cisc->ins_req(1,src);](https://github.com/openjdk/jdk/blob/6ed0ba2f8a2af58c45a6b7be684ef30d15af6ead/src/hotspot/share/opto/chaitin.cpp#L1729)
>>> 
>>> @jatin-bhateja would you agree that we always need such a edge to the spill-definition (`src`), and therefore instead of the `if` we should rather `assert` the condition (like my proposal)? If so, the question is what to do with the `stackSlotX` operations, as they do not lead to correct `oper_input_base` values, since in the mach-matching they do not use nodes such as `LoadX`, but only `stackSlotX` appears. This is checked in [MatchNode::needs_ideal_memory_edge](https://github.com/openjdk/jdk/blob/6ed0ba2f8a2af58c45a6b7be684ef30d15af6ead/src/hotspot/share/adlc/formssel.cpp#L3512). Would you agree that we need to make `MatchNode::needs_ideal_memory_edge` return 1 instead of 0? Should we simply add `stackSlotX` to this list, or should we make sure `stackSlotX` does not occur in the mach-matching, instead `LoadX`?
>>> 
>>> @jatin-bhateja A clarification question: do you want the additional spill-definintion (`src`) edge go to the CISC instruction (`cisc`), as it is done now, given the condition is fulfilled? Or do you want to have the additional edge go to the frame-pointer (`fp`)?
>> 
>> @eme64 , I think adding a precedence edge b/w SPILLED_DEF and CISC instruction after replacing SPILLE_DEFs with FP should add necessary data dependency constraint to enable generating a legal schedule.
>
>> @jatin-bhateja Great, let's add such a memory edge! How should I do that?
> 
> Please check [add_prec](https://github.com/openjdk/jdk/blob/cc598e03de39dd6e8d7e208a69d85b6a9cd0062f/src/hotspot/share/opto/node.hpp#L549) and its usage 
> https://github.com/openjdk/jdk/blob/cc598e03de39dd6e8d7e208a69d85b6a9cd0062f/src/hotspot/share/opto/output.cpp#L3057

@jatin-bhateja 
For all other cases (eg `addI`, `convI2L`, etc ) we are currently using
https://github.com/openjdk/jdk/blob/cc598e03de39dd6e8d7e208a69d85b6a9cd0062f/src/hotspot/share/opto/chaitin.cpp#L1729
This seems to add the dependencies before the inputs. But that depends on there being space before the inputs.
That is why we check `cisc->oper_input_base() > 1`
https://github.com/openjdk/jdk/blob/cc598e03de39dd6e8d7e208a69d85b6a9cd0062f/src/hotspot/share/opto/chaitin.cpp#L1727

All other cases where we have spilling (eg `addI`, `convI2L`, etc ), we have `cisc->oper_input_base() == 2`.
I think `_in[0]` is for control, and `_in[1]` for the memory edge (in the other cases not including `MoveF2I` etc).
`cisc->oper_input_base() == 2` in all other cases, because in `InstructForm::oper_input_base` we ask for `MatchNode::needs_ideal_memory_edge`.
In all cases except for `MoveF2I` etc, we say we need a memory edge there. Now we don't have space to set a memory edge before the inputs, and we simply do not set one.

So how do we work with this? Do I just remove `cisc->ins_req(1,src)` for all cases and alway use `add_req` as you have suggested? Or do I leave the other cases, and just make an else case and use `add_prec` there for `MoveF2I` etc? I am wondering what is a clean and consistent solution here.

Do you know why we add the memory edge before the inputs in the other cases? If I use `add_prec` then that adds the memory edge after the inputs, correct?

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

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