8332724: x86 MacroAssembler may over-align code

[djelinski]

The methods align32 and align64 are supposed to align the next instruction to the next 32 or 64 byte boundary using the minimum number of NOP bytes. However, when the target represented as a 32bit signed int is negative, the instructions generate 32 or 64 NOP bytes too many. This was observed in jbyte_disjoint_arraycopy_avx3 on a Linux machine, where a single align32 invocation generated 63 bytes of NOPs.

This PR addresses the problem by using bit operations to calculate the required number of bytes.

Tier1-3 tests passed.

On a side note, align64 and align32 instructions were meant for aligning data for use with zmm / ymm loads, but nowadays they are frequently used in places where align(CodeEntryAlignment) or align(OptoLoopAlignment) would be more appropriate. I can address that in a separate PR if you think it's worth fixing.

---

Progress

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

Warning

 ⚠️ Found leading lowercase letter in issue title for 8332724: x86 MacroAssembler may over-align code

Issue

• JDK-8332724: x86 MacroAssembler may over-align code (Bug - P5)

Reviewers

• Dean Long (@dean-long - Reviewer) ⚠️ Review applies to d0220193
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19353

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back djelinski! 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.

[openjdk]

@djelinski This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8332724: x86 MacroAssembler may over-align code

Reviewed-by: dlong, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 25 new commits pushed to the master branch:

• 0a9d1f8: 8332749: Broken link in MemorySegment.Scope.html
• c9a7b97: 8332829: [BACKOUT] C2: crash in compiled code because of dependency on removed range check CastIIs
• 7fd9d6c: 8332340: Add JavacBench as a test case for CDS
• 417d174: 8331348: Some incremental builds deposit files in the make directory
• 303ac9f: 8332671: Logging for pretouching thread stacks shows wrong memory range
• 90758f6: 8332808: Always set java.io.tmpdir to a suitable value in the build
• e19a421: 8332720: ubsan: instanceKlass.cpp:3550:76: runtime error: member call on null pointer of type 'struct Array'
• 2581935: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations
• b890336: 8328083: degrade virtual thread support for GetObjectMonitorUsage
• 4e6d851: 8325324: Implement JEP 477: Implicitly Declared Classes and Instance Main Methods (Third Preview)
• ... and 15 more: https://git.openjdk.org/jdk/compare/a0c5714dbc8a60d905f9deea153e7f31fbd64d06...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@djelinski 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.

[mlbridge]

Webrevs

• 02: Full - Incremental (6a7021a3)
• 01: Full - Incremental (d0220193)
• 00: Full (1786e1cb)

[dean-long]

Looks good.

[vnkozlov]

GHA x86-32 build issue:

/work/jdk/jdk/src/hotspot/cpu/x86/macroAssembler_x86.cpp: In member function ‘void MacroAssembler::align(uint)’:
/work/jdk/jdk/src/hotspot/cpu/x86/macroAssembler_x86.cpp:1162:18: error: comparison of integer expressions of different signedness: ‘uint’ {aka ‘unsigned int’} and ‘intx’ {aka ‘int’} [-Werror=sign-compare]
 1162 |   assert(modulus <= CodeEntryAlignment, "Alignment must be <= CodeEntryAlignment");
      |          ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~

[djelinski]

Thanks for pointing it out! Should be fixed now, will wait for GHA to confirm...

[vnkozlov]

Good.

We need to revisit types of flags which hold small values. intx and uintx are 64-bits values in 64-bits VM.

[djelinski]

Thanks for the reviews. Unless someone objects, I'll integrate this later today.

Re revisiting flag types, it seems that the only options currently available are bool, int, intx and uintx, other types might make jvmci unhappy.

[djelinski]

/integrate

[openjdk]

Going to push as commit 08d5100.
Since your change was applied there have been 52 commits pushed to the master branch:

• 97ee2ff: 8332416: Add more font selection options to Font2DTest
• 985b9ce: 8330694: Rename 'HeapRegion' to 'G1HeapRegion'
• 05f13e7: 8329667: [macos] Issue with JTree related fix for JDK-8317771
• 7bf1989: 8320575: generic type information lost on mandated parameters of record's compact constructors
• 253508b: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
• ebc520e: 8332841: GenShen: Pull shared members from control thread into common base class
• 236432d: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook
• b3b3366: 8332631: Update nsk.share.jpda.BindServer to don't use finalization
• f66a586: 8332641: Update nsk.share.jpda.Jdb to don't use finalization
• cd3e4c0: 8326734: text-decoration applied to lost when mixed with or
• ... and 42 more: https://git.openjdk.org/jdk/compare/a0c5714dbc8a60d905f9deea153e7f31fbd64d06...master

Your commit was automatically rebased without conflicts.

[openjdk]

@djelinski Pushed as commit 08d5100.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.