8342769: gcc port broken on HotSpot

Several areas in HotSpot are broken in the gcc port. These, with the exception of 1 rather big oversight within SharedRuntime::frem and SharedRuntime::drem, are all minor correctness issues within the code. These mostly can be fixed with simple changes to the code. Note that I am not sure whether the SharedRuntime::frem and SharedRuntime::drem fix is correct. It may be that they can be removed entirely

---

Progress

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8342769

Warning

 ⚠️ Found leading lowercase letter in issue title for 8342769: gcc port broken on HotSpot

Issue

• JDK-8342769: HotSpot gcc/win64 port is broken (Bug - P4) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21627/head:pull/21627
$ git checkout pull/21627

Update a local copy of the PR:
$ git checkout pull/21627
$ git pull https://git.openjdk.org/jdk.git pull/21627/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21627

View PR using the GUI difftool:
$ git pr show -t 21627

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21627.diff

Webrev

Link to Webrev Comment

:wave: Welcome back jwaters! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@TheShermanTanker This change is no longer ready for integration - check the PR body for details.

@TheShermanTanker The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Webrevs

• 16: Full (b93febb3)
• 15: Full (3f9ca206)
• 14: Full - Incremental (ff1c4664)
• 13: Full - Incremental (9ecb1d3c)
• 12: Full - Incremental (54930315)
• 11: Full - Incremental (027d494b)
• 10: Full - Incremental (37e0e4d7)
• 09: Full - Incremental (3fe38a6b)
• 08: Full - Incremental (6cf1c83a)
• 07: Full - Incremental (6caa759b)
• 06: Full - Incremental (8f57d674)
• 05: Full - Incremental (b0083f8c)
• 04: Full - Incremental (967b9606)
• 03: Full - Incremental (47810433)
• 02: Full - Incremental (fb171a43)
• 01: Full - Incremental (12e82f68)
• 00: Full (541d4f89)

The sharedRuntime stuff is tricky and messy. Not sure about what is best/right/workable there. Maybe we even need to factor out like ./os/windows/sharedRuntimeRem.cpp?

I'm not sure what is best too. I think we might need more people looking at this, in particular Windows ARM64 maintainers, because they might be the only platform that this change will affect. Some in the mailing lists mentioned that fmod_winx64 might not be needed anymore and could be removed entirely

Bumping, please advise how to proceed on the sharedRuntime issue. It potentially affects Windows/ARM64 in an unknown way

You need to contact the Windows-Aarch64 port maintainers and get them to tell you what the correct thing for Aarch64 is. Paging @mo-beck , @luhenry and @lewurm (https://openjdk.org/jeps/388)

Passing onto Microsoft's VM Engineering manager @brianjstafford - he'll find someone to take a look and advise.

@swesonga, can you please take a look at this?

Bumping

Bumping

Reviewing this change today. cc @brianjstafford

Bumping

@swesonga is out of office this week and will resume looking into this when he's back.

I've changed the check to instead define the constants when _M_ARM64 is defined. This is the cleanest approach I can come up with, and doesn't touch the workaround code in frem and drem either

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

You mean like how x86 has its own custom fmod implementation in the assembler? That does sound pretty good to me, then we could get rid of this workaround entirely, but I don't know how to implement this on ARM64

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

You mean like how x86 has its own custom fmod implementation in the assembler? That does sound pretty good to me, then we could get rid of this workaround entirely, but I don't know how to implement this on ARM64

Oh, OK. I'll have a look.

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

You mean like how x86 has its own custom fmod implementation in the assembler? That does sound pretty good to me, then we could get rid of this workaround entirely, but I don't know how to implement this on ARM64

Oh, OK. I'll have a look.

I had a look at SharedRuntime::fmod_winx64. I'm sure it's fine for AArch64. Just put it in os_windows.

Wouldn't it be far easier to handle fmod ourselves in the AArch64b back end?

You mean like how x86 has its own custom fmod implementation in the assembler? That does sound pretty good to me, then we could get rid of this workaround entirely, but I don't know how to implement this on ARM64

Oh, OK. I'll have a look.

I had a look at SharedRuntime::fmod_winx64. I'm sure it's fine for AArch64. Just put it in os_windows.

I'm not sure I understand - The fix in this Pull Request already uses SharedRuntime::fmod_winx64 (Or more accurately, it doesn't change the existing code that uses it, just makes the ifdef check more accurate). I don't know if moving the implementation from sharedRuntimeRem.cpp to os_windows.cpp gives any benefit, and sharedRuntimeRem.cpp is already under os/windows. I might have misunderstood you earlier, but it sounded as if you were proposing to implement frem and drem in assembly for ARM64 like what is already done for x86

I'm not sure I understand - The fix in this Pull Request already uses SharedRuntime::fmod_winx64 (Or more accurately, it doesn't change the existing code that uses it, just makes the ifdef check more accurate). I don't know if moving the implementation from sharedRuntimeRem.cpp to os_windows.cpp gives any benefit, and sharedRuntimeRem.cpp is already under os/windows. I might have misunderstood you earlier, but it sounded as if you were proposing to implement frem and drem in assembly for ARM64 like what is already done for x86

I got horribly confused.

I thought about custome asm, but there's little point in doing so because fmod is always going to require repeated division and subtraction, so it's going to be slow.

What you're doing in this patch looks correct as far as it goes, but amd64and x64 in the comments in sharedRuntime.[ch]pp is wrong. Please find and fix those to make it clear we need this function on on AArch64 too. The name of the function SharedRuntime::fmod_winx64 is at least misleading if not wrong. Maybe SharedRuntime::fmod_win64?

My apologies for the confusion.

Bump

Bumping

@TheShermanTanker - Is there someone in particular you're waiting to approve this PR? I think you can re-request reviews.

@karianna I need 2 reviews with at least 1 of them being from a reviewer before this is marked by the automated systems as allowed for integration - Skara doesn't count swesonga's review because he's apparently not registered in the census (I think there might be a mistake there, either in the Skara tool or someone forgot to register him). I guess David or Andrew, or any one of the people from Microsoft who have seen this PR are the next best people to approve this. I had forgotten completely about the review request feature, thanks for reminding me :)

I had forgotten completely about the review request feature, thanks for reminding me :)

Only works if you have Github notifications enabled (I don't) else you have to return to the PR to see it.

So for the sharedRuntime changes, IIUC what was previously only needed for Windows x64 is now only needed for Windows Aarch64 - correct?

Yes, that seems to be the case

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Keep open please

@TheShermanTanker - Are you needing further reviews?

@TheShermanTanker - Are you needing further reviews?

I think this doesn't need further review, but I'm waiting for all the corresponding unused variable Pull Requests for the other areas of the JDK to get the green light before I integrate them all at once