RFR: 8288467: remove memory_operand assert for spilled instructions [v2]

[Jatin Bhateja]

On Mon, 20 Jun 2022 08:30:26 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> In [JDK-8282555](https://bugs.openjdk.org/browse/JDK-8282555) I added this assert, because on x64 this seems to always hold. But it turns out there are instructions on x86 (32bit) that violate this assumption.
>> 
>> **Why it holds on x64**
>> It seems we only ever do one read or one write per instruction. Instructions with multiple memory operands are extremely rare.
>> 1. we spill from register to memory: we land in the if case, where the cisc node has an additional input slot for the memory edge.
>> 2. we spill from register to stackSlot: no additional input slot is reserved, we land in else case and add an additional precedence edge.
>> 
>> **Why it is violated on x86 (32bit)**
>> We have additional cases that land in the else case. For example spilling `src1` from `addFPR24_reg_mem` to `addFPR24_mem_cisc`.
>> https://urldefense.com/v3/__https://github.com/openjdk/jdk19/blob/53bf1bfdabb79b37afedd09051d057f9eea620f2/src/hotspot/cpu/x86/x86_32.ad*L10325-L10327__;Iw!!ACWV5N9M2RV99hQ!PYxlwoaWol7yzkAZPpyEMAZ8edamNWgRAi97PDR6JEhEdgrozu2eHCWKrlJikuxd5swedbPlrBUKokFh2qIq7YgyoL0_WDER$  https://urldefense.com/v3/__https://github.com/openjdk/jdk19/blob/53bf1bfdabb79b37afedd09051d057f9eea620f2/src/hotspot/cpu/x86/x86_32.ad*L10368-L10370__;Iw!!ACWV5N9M2RV99hQ!PYxlwoaWol7yzkAZPpyEMAZ8edamNWgRAi97PDR6JEhEdgrozu2eHCWKrlJikuxd5swedbPlrBUKokFh2qIq7YgyoF8ojvq1$ 
>> We land in the else case, because both have 2 inputs, thus `oper_input_base() == 2`.
>> And both have memory operands, so the assert must fail.
>> 
>> **Solutions**
>> 1. Remove the Assert, as it is incorrect.
>> 2. Extend the assert to be correct.
>>    - case 1: reg to mem spill, where we have a reserved input slot in cisc for memory edge
>>    - case 2: reg to stackSlot spill, where both mach and cisc have no memory operand.
>>    - other cases, with various register, stackSlot and memory inputs and outputs. We would have to find a general rule, and test it properly, which is not trivial because getting registers to spill is not easy to precisely provoke.
>> 3. Have platform dependent asserts. But also this makes testing harder.
>> 
>> For now I went with 1. as it is simple and as far as I can see correct.
>> 
>> Running tests on x64 (should not fail). **I need someone to help me with testing x86 (32bit)**. I only verified the reported test failure with a 32bit build.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update src/hotspot/share/opto/chaitin.cpp
>   
>   fix typo
>   
>   Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>

Marked as reviewed by jbhateja (Committer).

Hi @eme64,
Thanks for pointing this out. Original bug was related to incorrect scheduling of CISC node with just the stack operand which was circumvented by adding a precedence edge from incoming spill copy node. In general there is always a 1-1 correspondence b/w memory_operand index of CISC instruction and cisc_operand index of non-CISC counterpart.

As you mentioned there are not many cases where an instruction accept more than one memory operand.

Following patterns in x86_32.ad (which are relevant to SSE targets) are the only occurrences accepting more than one memory operand.
 
instruct addFPR24_mem_cisc(stackSlotF dst, memory src1, memory src2) %{
instruct addFPR24_mem_mem(stackSlotF dst, memory src1, memory src2) %{
instruct mulFPR24_mem_mem(stackSlotF dst, memory src1, memory src2) %{

And for each of these a constant -1 value is returned as memory_operand currently. 

 const MachOper* addFPR24_mem_ciscNode::memory_operand() const { return (MachOper*)-1; }
const MachOper* addFPR24_mem_cisc_0Node::memory_operand() const { return (MachOper*)-1; }
const MachOper* addFPR24_mem_memNode::memory_operand() const { return (MachOper*)-1; }
const MachOper* mulFPR24_mem_memNode::memory_operand() const { return (MachOper*)-1; }

Post matcher operand array is sacrosanct, only problem here is that oper_input_base() for **non-cisc counterparts** are 2 since first input is memory edge.  Due to this it enters the control flow having assertion check.  

Your fix to remove assertion, looks safe to me.

Best Regards,
Jatin

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

PR: https://git.openjdk.org/jdk19/pull/33