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

[Emanuel Peter]

On Thu, 31 Mar 2022 05:49:47 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> @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?
>
>> 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?
> 
> Currently memory edges are being added for instructions which directly access memory,  ADLC enforces this by scanning through Ideal nodes of a matcher pattern in top-down manner. 
> Almost all the machine nodes decorated with **Flag_is_cisc_alternate** flag access Load/Store IR in their selection patterns. Only exceptions[1][2][3] as you pointed out are the ones which perform load/store from stack locations. Thus all I am suggesting is for all such cases without doing many changes we can add the DEF_Spill precedence edge which gets added after all the inputs but will still constrain the scheduling order. 
> 
> Thus an instructions which has a CISC alternate but lacks memory_operand can be handled by adding a prescience edge.
> 
>  [1] MoveF2I_stack_regNode() {  _num_opnds = 2; _opnds = _opnd_array; init_flags(Flag_is_cisc_alternate | Flag_needs_anti_dependence_check); }
>  [2] MoveI2F_stack_regNode() {  _num_opnds = 2; _opnds = _opnd_array; init_flags(Flag_is_cisc_alternate | Flag_needs_anti_dependence_check); }
>  [3] MoveD2L_stack_regNode() {  _num_opnds = 2; _opnds = _opnd_array; init_flags(Flag_is_cisc_alternate | Flag_needs_anti_dependence_check); }

@jatin-bhateja thanks for bringing up the assertion, I implemented and ran many tests.
@vnkozlov @TobiHartmann Is this also ok with you?

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

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