On Mon, 28 Oct 2024 18:15:38 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > src/hotspot/cpu/x86/interpreterRT_x86_32.cpp line 47: > >> 45: #ifdef AMD64 >> 46: #ifdef _WIN64 >> 47: // FIXME: This is weird. How can we ever have _WIN64 for 32-bit code? >> I wonder what was meant. /ihse > > I think this piece of code will never get compiled and should be removed, and > just the `#else` clause kept, but I guess some code archaeology is in place > to figure out how and why this was added in the first place. I think this is a copy-paste error from [JDK-8199809](https://bugs.openjdk.org/browse/JDK-8199809): the code from `interpreterRT_x86_64.cpp` (where `WIN64` makes sense) was copy-pasted here in `interpreterRT_x86_32.cpp`. In fact, `AMD64` in `interpreterRT_x86_64.cpp` makes no sense as well. I'll clean it up: [JDK-8343167](https://bugs.openjdk.org/browse/JDK-8343167). > src/hotspot/cpu/x86/sharedRuntime_x86_32.cpp line 1478: > >> 1476: int frame_complete = ((intptr_t)__ pc()) - start; >> 1477: >> 1478: // FIXME: The logic below do not apply anymore. Should we change >> anything? /ihse > > This file is now Linux only, so we should be able to remove any Windows > special code. Someone with better knowledge about the product needs to > confirm that the comment is indeed correct, and that this was only needed on > Windows. Nah, leave it as is. Let's not regress native stubs unnecessarily, and this whole file would be gone after we deprecate 32-bit port completely. > src/hotspot/cpu/x86/sharedRuntime_x86_32.cpp line 1714: > >> 1712: __ restore_cpu_control_state_after_jni(noreg); >> 1713: >> 1714: // FIXME: The logic below do not apply anymore. Should we change >> anything? /ihse > > Same here as above. Same reply as above :) > src/hotspot/cpu/x86/x86_32.ad line 3715: > >> 3713: %} >> 3714: >> 3715: // FIXME: The logic below do not apply anymore. Should we change >> anything? /ihse > > Here too we don't need Windows-specific support, since this is Linux only. > But I need confirmation that the comment is correct so this code is really > just Windows-specific. It looks like it is a dusty corner case. But the same logic as above applies: let's not touch it, and instead wait for it to go away with the remaining bits of 32-bit x86 port. I see `eRegP_no_EBP` is used for safepoint polls, so if we are wrong about the scope of this, rewriting these match rules to just `eRegP` might introduce surprising regressions. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1819606745 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1819611536 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1819612008 PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1819617067
