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

[Jatin Bhateja]

On Mon, 28 Mar 2022 02:21:34 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>>> This can lead to reversed scheduling, where we read from a stackSlot before we wrote to it, leading to wrong results.
>> 
>> Scheduler is free to move around instructions without explicit control/memory edges but under all circumstances it should still honor USE-DEF constrain. Following code[1] explicitly connects stack spilled definition to its user if CISC variant of user is available and this input[2] is later replaced by frame pointer[3] when CISC instruction gets created during fixup_spill, this does not disturb the semantics since instruction is still able read correct value from stack address emitted for stackOperands.
>> 
>> This seems to be the root cause of the problem as it disconnects original schedule constraining definition from its user. I think having an extra edge for frame pointer for CISC instructions instead of replacing spill definition edge may guide the scheduler to emit legal schedule. 
>> 
>> [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/reg_split.cpp#L254
>> [2] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/chaitin.cpp#L1707
>> [3] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/chaitin.cpp#L1726
>
>> @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

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

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