RFR: 8288467: remove memory_operand assert for spilled instructions [v2]
Emanuel Peter
epeter at openjdk.org
Mon Jun 20 08:30:26 UTC 2022
> 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!Ja7XHQ8ZO-anMMYwAeFOBe0xB-O89jTk4EoBX3gG31OH_x2LE0n41wAJ85n0nS_N5new2tsRnkwJA1sTJ9fh9rVqPXngoA$ https://urldefense.com/v3/__https://github.com/openjdk/jdk19/blob/53bf1bfdabb79b37afedd09051d057f9eea620f2/src/hotspot/cpu/x86/x86_32.ad*L10368-L10370__;Iw!!ACWV5N9M2RV99hQ!Ja7XHQ8ZO-anMMYwAeFOBe0xB-O89jTk4EoBX3gG31OH_x2LE0n41wAJ85n0nS_N5new2tsRnkwJA1sTJ9fh9rVHaV4ngA$
> 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>
-------------
Changes:
- all: https://git.openjdk.org/jdk19/pull/33/files
- new: https://git.openjdk.org/jdk19/pull/33/files/d082aa91..951e91de
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk19&pr=33&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk19&pr=33&range=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk19/pull/33.diff
Fetch: git fetch https://git.openjdk.org/jdk19 pull/33/head:pull/33
PR: https://git.openjdk.org/jdk19/pull/33
More information about the hotspot-compiler-dev
mailing list