8321648: Integral gather optimized mask computation.

[jatin-bhateja]

Hi,

This bug fix patch optimizes integral gather mask computation using cheaper instruction and fixes incorrect instruction attributes in legacy integral gather instructions.

All Vector API JTREG tests are passing with this at various AVX levels.

Kindly review and share feedback.

Best Regards,
Jatin

---

Progress

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

Issue

• JDK-8321648: Integral gather optimized mask computation. (Bug - P4)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Sandhya Viswanathan (@sviswa7 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17048

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jbhateja! 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]

@jatin-bhateja The following label will be automatically applied to this pull request:

• hotspot-compiler

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

• 00: Full (464362a4)

[merykitty]

While you are at it, you can change the address operand of these to only accept no-index ones, removing the need of the lea instruction.

[jatin-bhateja]

While you are at it, you can change the address operand of these to only accept no-index ones, removing the need of the lea instruction.

Hi @merykitty , Memory patterns fold address generation components (base , index, scale) into instruction encoding thus eliminating a need to emit explicit ADD, MUL instruction sequence to compute address, saving lea may prevent folding memory patterns and may prove to be costly.

[TobiHartmann]

@jatin-bhateja Could you elaborate on what the failure mode for the incorrect instruction attribution would look like? Is this just inefficient execution or would it lead to a crash?

[jatin-bhateja]

@jatin-bhateja Could you elaborate on what the failure mode for the incorrect instruction attribution would look like? Is this just inefficient execution or would it lead to a crash?

Hi @TobiHartmann , These gather instruction are strictly applicable for AVX2 targets and will always be VEX encoded, instruction patterns corresponding to them operate on legacy vector register mask operands. Thus, this looks more of a typo error to set VL as true. Other change is for strength reduction and replacing a memory operand instruction.

[TobiHartmann]

Thanks for the clarification, Jatin. So the incorrect encoding has no real (negative) effect on code generation?

[jatin-bhateja]

Thanks for the clarification, Jatin. So the incorrect encoding has no real (negative) effect on code generation?

Instruction attributes VL allows EVEX to VEX demotions, iff participating vectors are allocated from lower register bank and are less than 512 bit wide. On the contrary, if RA makes an allocation from higher register bank then we may need EVEX bits to accomodate encoding for a reigister in higher register bank even if vectors are lesser than 512 bit wide.

However, there are some instructions which are constrained to use VEX encoding and AVX2 gather belong to that class. Patch also strength reduces mask computation which is currently using memory operands.

[TobiHartmann]

Okay, so if I understand correctly, since these instructions are always VEX encoded, the VL set to true does not make a difference and should not lead to failures, correct?

Looks reasonable to me but someone more experienced with this should have a look as well.

/reviewers 2

[openjdk]

@jatin-bhateja 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:

8321648: Integral gather optimized mask computation.

Reviewed-by: thartmann, sviswanathan

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 78 new commits pushed to the master branch:

• 459957f: 8322062: com/sun/jdi/JdwpAllowTest.java does not performs negative testing with prefix length
• b98d13f: 8259637: java.io.File.getCanonicalPath() returns different values for same path
• 4f3de09: 8321940: Improve CDSHeapVerifier in handling of interned strings
• 1fde8b8: 8321933: TestCDSVMCrash.java spawns two processes
• 66aeb89: 8315462: [REDO] runtime/NMT/SummarySanityCheck.java failed with "Total committed (MMMMMM) did not match the summarized committed (NNNNNN)"
• a5122d7: 8314029: Add file name parameter to Compiler.perfmap
• c0a3b76: 8316197: Make tracing of inline cache available in unified logging
• 7e1d26d: 8322287: Parallel: Remove unused arg in adjust_eden_for_pause_time and adjust_eden_for_minor_pause_time
• 5584ba3: 8322097: Serial: Refactor CardTableRS::find_first_clean_card
• 75d382d: 8322204: Parallel: Remove unused _collection_cost_margin_fraction
• ... and 68 more: https://git.openjdk.org/jdk/compare/ce108446ca1fe604ecc24bbefb0bf1c6318271c7...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]

@TobiHartmann
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[jatin-bhateja]

Okay, so if I understand correctly, since these instructions are always VEX encoded, the VL set to true does not make a difference and should not lead to failures, correct?

Thanks @TobiHartmann , yes, that what my understanding is.
A purely vex encoded instruction do not have sufficient bits in VEX prefix to encode a register from a higher register bank.

[sviswa7]

LGTM

[jatin-bhateja]

/integrate

[openjdk]

Going to push as commit 76637c5.
Since your change was applied there have been 80 commits pushed to the master branch:

• 59073fa: 8322154: RISC-V: JDK-8315743 missed change in MacroAssembler::load_reserved
• 808a039: 8321815: Shenandoah: gc state should be synchronized to java threads only once per safepoint
• 459957f: 8322062: com/sun/jdi/JdwpAllowTest.java does not performs negative testing with prefix length
• b98d13f: 8259637: java.io.File.getCanonicalPath() returns different values for same path
• 4f3de09: 8321940: Improve CDSHeapVerifier in handling of interned strings
• 1fde8b8: 8321933: TestCDSVMCrash.java spawns two processes
• 66aeb89: 8315462: [REDO] runtime/NMT/SummarySanityCheck.java failed with "Total committed (MMMMMM) did not match the summarized committed (NNNNNN)"
• a5122d7: 8314029: Add file name parameter to Compiler.perfmap
• c0a3b76: 8316197: Make tracing of inline cache available in unified logging
• 7e1d26d: 8322287: Parallel: Remove unused arg in adjust_eden_for_pause_time and adjust_eden_for_minor_pause_time
• ... and 70 more: https://git.openjdk.org/jdk/compare/ce108446ca1fe604ecc24bbefb0bf1c6318271c7...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jatin-bhateja Pushed as commit 76637c5.

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