8303762: Optimize vector slice operation with constant index using VPALIGNR instruction

[jatin-bhateja]

Patch optimizes Vector. slice operation with constant index using x86 ALIGNR instruction.
It also adds a new hybrid call generator to facilitate lazy intrinsification or else perform procedural inlining to prevent call overhead and boxing penalties in case the fallback implementation expects to operate over vectors. The existing vector API-based slice implementation is now the fallback code that gets inlined in case intrinsification fails.

Idea here is to add infrastructure support to enable intrinsification of fast path for selected vector APIs, else enable inlining of fall-back implementation if it's based on vector APIs. Existing call generators like PredictedCallGenerator, used to handle bi-morphic inlining, already make use of multiple call generators to handle hit/miss scenarios for a particular receiver type. The newly added hybrid call generator is lazy and called during incremental inlining optimization. It also relieves the inline expander to handle slow paths, which can easily be implemented library side (Java).

Vector API jtreg tests pass at AVX level 2, remaining validation in progress.

Performance numbers:

System : 13th Gen Intel(R) Core(TM) i3-1315U

Baseline:
Benchmark                                                (size)   Mode  Cnt      Score   Error   Units
VectorSliceBenchmark.byteVectorSliceWithConstantIndex1     1024  thrpt    2   9444.444          ops/ms
VectorSliceBenchmark.byteVectorSliceWithConstantIndex2     1024  thrpt    2  10009.319          ops/ms
VectorSliceBenchmark.byteVectorSliceWithVariableIndex      1024  thrpt    2   9081.926          ops/ms
VectorSliceBenchmark.intVectorSliceWithConstantIndex1      1024  thrpt    2   6085.825          ops/ms
VectorSliceBenchmark.intVectorSliceWithConstantIndex2      1024  thrpt    2   6505.378          ops/ms
VectorSliceBenchmark.intVectorSliceWithVariableIndex       1024  thrpt    2   6204.489          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex1     1024  thrpt    2   1651.334          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex2     1024  thrpt    2   1642.784          ops/ms
VectorSliceBenchmark.longVectorSliceWithVariableIndex      1024  thrpt    2   1474.808          ops/ms
VectorSliceBenchmark.shortVectorSliceWithConstantIndex1    1024  thrpt    2  10399.394          ops/ms
VectorSliceBenchmark.shortVectorSliceWithConstantIndex2    1024  thrpt    2  10502.894          ops/ms
VectorSliceBenchmark.shortVectorSliceWithVariableIndex     1024  thrpt    2   9756.573          ops/ms

With opt:
Benchmark                                                (size)   Mode  Cnt      Score   Error   Units
VectorSliceBenchmark.byteVectorSliceWithConstantIndex1     1024  thrpt    2  34122.435          ops/ms
VectorSliceBenchmark.byteVectorSliceWithConstantIndex2     1024  thrpt    2  33281.868          ops/ms
VectorSliceBenchmark.byteVectorSliceWithVariableIndex      1024  thrpt    2   9345.154          ops/ms
VectorSliceBenchmark.intVectorSliceWithConstantIndex1      1024  thrpt    2   8283.247          ops/ms
VectorSliceBenchmark.intVectorSliceWithConstantIndex2      1024  thrpt    2   8510.695          ops/ms
VectorSliceBenchmark.intVectorSliceWithVariableIndex       1024  thrpt    2   5626.367          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex1     1024  thrpt    2    960.958          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex2     1024  thrpt    2   4155.801          ops/ms
VectorSliceBenchmark.longVectorSliceWithVariableIndex      1024  thrpt    2   1465.953          ops/ms
VectorSliceBenchmark.shortVectorSliceWithConstantIndex1    1024  thrpt    2  32748.061          ops/ms
VectorSliceBenchmark.shortVectorSliceWithConstantIndex2    1024  thrpt    2  33674.408          ops/ms
VectorSliceBenchmark.shortVectorSliceWithVariableIndex     1024  thrpt    2   9346.148          ops/ms

Please share your feedback.

Best Regards,
Jatin

---

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

Issue

• JDK-8303762: Optimize vector slice operation with constant index using VPALIGNR instruction (Enhancement - P4)

Reviewers

• Xiaohong Gong (@XiaohongGong - Committer)
• Eric Fang (@erifan - Author) 🔄 Re-review required (review applies to ae242926)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24104

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

Using diff file

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

Using 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

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

• core-libs
• graal
• hotspot

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

[jatin-bhateja]

/label add hotspot-compiler-dev

[openjdk]

@jatin-bhateja
The hotspot-compiler label was successfully added.

[bridgekeeper]

@jatin-bhateja This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jatin-bhateja]

/keepalive

[openjdk]

@jatin-bhateja The pull request is being re-evaluated and the inactivity timeout has been reset.

[openjdk]

@jatin-bhateja this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8303762
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[bridgekeeper]

@jatin-bhateja This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jatin-bhateja]

Performance after AVX2 backend modifications

Benchmark                                                (size)   Mode  Cnt      Score   Error   Units
VectorSliceBenchmark.byteVectorSliceWithConstantIndex1     1024  thrpt    2  51644.530          ops/ms
VectorSliceBenchmark.byteVectorSliceWithConstantIndex2     1024  thrpt    2  48171.079          ops/ms
VectorSliceBenchmark.byteVectorSliceWithVariableIndex      1024  thrpt    2   9662.306          ops/ms
VectorSliceBenchmark.intVectorSliceWithConstantIndex1      1024  thrpt    2  14358.347          ops/ms
VectorSliceBenchmark.intVectorSliceWithConstantIndex2      1024  thrpt    2  14619.920          ops/ms
VectorSliceBenchmark.intVectorSliceWithVariableIndex       1024  thrpt    2   6675.824          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex1     1024  thrpt    2    818.911          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex2     1024  thrpt    2   4778.321          ops/ms
VectorSliceBenchmark.longVectorSliceWithVariableIndex      1024  thrpt    2   1612.264          ops/ms
VectorSliceBenchmark.shortVectorSliceWithConstantIndex1    1024  thrpt    2  35961.146          ops/ms
VectorSliceBenchmark.shortVectorSliceWithConstantIndex2    1024  thrpt    2  39072.170          ops/ms
VectorSliceBenchmark.shortVectorSliceWithVariableIndex     1024  thrpt    2  11209.685          ops/ms

[mlbridge]

Webrevs

• 14: Full - Incremental (9625b04e)
• 13: Full - Incremental (444a3568)
• 12: Full (1dfff558)
• 11: Full - Incremental (ae242926)
• 10: Full - Incremental (2c7eb96d)
• 09: Full (4b807102)
• 08: Full (9da1f862)
• 07: Full - Incremental (340f1849)
• 06: Full - Incremental (70c22932)
• 05: Full - Incremental (d55fa594)
• 04: Full - Incremental (f36ae6dd)
• 03: Full - Incremental (405de56f)
• 02: Full (e7c7374b)
• 01: Full - Incremental (04be59a6)
• 00: Full (b2e93434)

[jatin-bhateja]

Hi @XiaohongGong , your comments have been addressed.
Hi @sviswa7, can you kindly review x86 part.

[XiaohongGong]

LGTM! Thanks for your updating!

[sviswa7]

Hi @XiaohongGong , your comments have been addressed. Hi @sviswa7, can you kindly review x86 part.

Thanks @jatin-bhateja. I will take a look next week.

[jatin-bhateja]

Hi @sviswa7 , Please let me know if you have comments on x86 backend part.

[jatin-bhateja]

Hi @sviswa7, @merykitty Can you please review x86 backed implementation.

[merykitty]

I still think it is not a good solution to add an intrinsic method for this operation. We should add constant info to TypeVect and transform the slice pattern into VectorSliceNode. I think it is adequate to add bit info (zeros and ones) to each TypeVect instance so that we can do decent inference without too much additional memory overhead.

[merykitty]

This may be problematic if the intrinsification does not succeed because the arguments have not been constant-folded. It is because the order in which methods are processed during incremental inline is not deterministic.

[jatin-bhateja]

Hi @merykitty , Intrinsicification failure due to any such reason is same with and without this patch, in case of slice intrinsic failure we simply inline the fallback implementation which is comprised of vector APIs, VectorSupport* entry points of APIs should then go through intrinsification attempts independently and may succeeded or fail if constraints are not met.

[merykitty]

CallStaticJavaNode::Ideal will enqueue the call for incremental inline again when it is invoked. That means if the method fails to get intrinsified at first, then its arguments constant-fold, then the intrinsification may succeed upon retry.

[jatin-bhateja]

I think there is no harm in-lining fallback after first un-successful attempt of intrinsification for sliceOp, as fallback is composed of vectorAPI and we are giving them opportunity for intrinsificaiton, this save costly boxing operation and performance will be at par with what we have today. WDYT ?

[merykitty]

But this will affect other intrinsics, too, they are not implemented using other vector API operations.

[jatin-bhateja]

We only perform fallback inlining on first intrinsification failure for sliceOp, this is a very localized change.
https://github.com/jatin-bhateja/jdk/blob/1dfff5589c8b6c83dfc9810bddbb676c7982c904/src/hotspot/share/opto/callGenerator.cpp#L455

[merykitty]

Thanks for pointing out the change. I think that's more hacky than I have expected.

[jatin-bhateja]

This is selective enablement of inlining for intrinsic failures which uses vector API in the fall back implimentation.

[jatin-bhateja]

I still think it is not a good solution to add an intrinsic method for this operation. We should add constant info to TypeVect and transform the slice pattern into VectorSliceNode. I think it is adequate to add bit info (zeros and ones) to each TypeVect instance so that we can do decent inference without too much additional memory overhead.

What you are asking for is a bigger generic change which can be taken up as a separate RFE once this is committed.

[merykitty]

What you are asking for is a bigger generic change which can be taken up as a separate RFE once this is committed.

I think there is no need to rush this functionality, and this will become unnecessary when TypeVect can track const-ness of the value, so we should not proceed with this change.

[jatin-bhateja]

What you are asking for is a bigger generic change which can be taken up as a separate RFE once this is committed.

I think there is no need to rush this functionality, and this will become unnecessary when TypeVect can track const-ness of the value, so we should not proceed with this change.

Slice operations are used in simdjson UtfValidator and its better to push this patch rather than holding it for some future optimization.
simdjson/simdjson-java#68

[merykitty]

I don't agree with this change because the benefit is little, the intrinsic does not stand in the future, and the implementation is hacky and not trivial. I think I can accept this PR if:

• It can be proved that the intrinsic is necessary even when we have constant information for TypeVect.
• The fall back inlining can be made more generally applicable, and it is reliable under the non-determinism of incremental inlining.

[jatin-bhateja]

I don't agree with this change because the benefit is little, the intrinsic does not stand in the future, and the implementation is hacky and not trivial. I think I can accept this PR if:

• It can be proved that the intrinsic is necessary even when we have constant information for TypeVect.
• The fall back inlining can be made more generally applicable, and it is reliable under the non-determinism of incremental inlining.

Hi @merykitty,

Thank you for the detailed feedback. I’ve looked into the alternative you suggested—adding constant/lane info to TypeVect and having the compiler recognize the slice pattern (rearrange + blend with constant shuffle and mask) and replace it with VectorSliceNode, without a dedicated intrinsic—and I’d like to explain why I still believe the hybrid call generator is the more practical choice for this change, while keeping the door open for the pattern-based approach later.

Why the pattern-based approach is still very non-trivial

Even after we have constant shuffle and constant mask recognizing the slice idiom and recovering origin remains highly non-trivial:

1. The shuffle is not a single constant node. It is the output of iotaShuffle(origin, 1, true), which is implemented by several nodes in the IR. So we must match that iotaShuffle subgraph and find the node whose input is the scalar origin (and require that to be ConI),

2. The mask is compare-with-constant (e.g. iota.compare(LT, filter) with filter = vlen - origin) .

3. **Actual patten match involving constant shuffle / mask ** depends on getting the blend/rearrange wiring right (which input is vec1 vs vec2, both rearranges using the same shuffle, mask semantics matching slice).

So even with TypeVect constant info, we still need very complex / non-trivial pattern match probable the biggest pattern match in idiealization

Why the hybrid call generator is a better fit for this change

• Bounded complexity: We have one clear boundary—the slice call—and one check—origin->is_con(). If true, we emit VectorSliceNode; otherwise we inline the Java fallback. No graph pattern matching, no decoding of shuffle/mask vectors, no dependency on the exact shape of iotaShuffle or compare nodes.

• Same user benefit when it works: In both approaches, constant-index slice gets VectorSliceNode → VPALIGNR. With the hybrid approach, if we don’t intrinsify (e.g. origin not yet constant), we still inline the fallback and the subsequent Vector API calls can be optimized as today.

Proposed path

I think the most practical path is to proceed with the hybrid call generator for this PR so we can ship a reliable slice optimization (including for use cases like simdjson) without taking on the full cost and risk of TypeVect constant info and slice-pattern matching right now. Once this is in, we have a clear baseline: constant-index slice is optimized; variable index uses the inlined fallback.

If we later introduce TypeVect constant info and a more generic framework for recognizing vector idioms (e.g. patterns that consume constant shuffle/mask or that match subgraphs like iotaShuffle), we can revisit slice: add a pattern-based recognition that either supplements or eventually replaces the intrinsic. Please note x86 back end implimentation will still be usable if we later remove intrinsic and use complex patten matching to deduce VectorSliceNode

Thanks again for the review.

[merykitty]

Even after we have constant shuffle and constant mask recognizing the slice idiom and recovering origin remains highly non-trivial

I think you are misunderstanding, when TypeVect has constant information, the shuffle indices and the blend mask will constant fold, and there is no iotaShuffle subgraph or the compare-with-constant node there to inspect to extract the origin.

[jatin-bhateja]

Even after we have constant shuffle and constant mask recognizing the slice idiom and recovering origin remains highly non-trivial

I think you are misunderstanding, when TypeVect has constant information, the shuffle indices and the blend mask will constant fold, and there is no iotaShuffle subgraph or the compare-with-constant node there to inspect to extract the origin.

Given that vector lane itself are variable an application of constant shuffle will not fold rearrange, so a complex pattern match to infer VectorSliceNode (with constant index) probably in blend idealization will still have to deal with a graph pallet consisting of blend with constant mask, two re-arrange nodes with constant shuffles, and some other non-folded nodes.

    /*package-private*/
    final
    @ForceInline
    IntVector sliceTemplate(int origin, Vector<Integer> v1) {
        IntVector that = (IntVector) v1;
        that.check(this);
        Objects.checkIndex(origin, length() + 1);
        IntVector iotaVector = (IntVector) iotaShuffle().toBitsVector();
        IntVector filter = broadcast((int)(length() - origin));
        VectorMask<Integer> blendMask = iotaVector.compare(VectorOperators.LT, filter);
        AbstractShuffle<Integer> iota = iotaShuffle(origin, 1, true);
        return that.rearrange(iota).blend(this.rearrange(iota), blendMask);
    }

I think its reasonable to proceed with the hybrid call generator for this PR so we can ship a reliable slice optimization (including for use cases like simdjson) without taking on the full cost and risk of TypeVect constant info and slice-pattern matching right now. In future once constant TypeVect are in production we can replace existing intrinsification approach with patten match and still use the backend implementation as it is.

Lets think over it and also seek other's opinion (@sviswa7)

[XiaohongGong]

I’m fine with using the more straightforward approach to intrinsify the slice API when the origin is a constant. In my view, this could also benefit other APIs and future optimizations (for example, #28520), since slice is a general vector operation. Relying on pattern matching makes the compiler implementation significantly more complex in my opinion.

Regarding inlining of the fallback implementation, I think we do need such a mechanism to handle APIs that fail to inline on the first attempt, given that the current fallback overhead is much heavier and leads to worse performance. And I agree with @merykitty that a more generic solution would be more preferable.

[jatin-bhateja]

I’m fine with using the more straightforward approach to intrinsify the slice API when the origin is a constant. In my view, this could also benefit other APIs and future optimizations (for example, #28520), since slice is a general vector operation. Relying on pattern matching makes the compiler implementation significantly more complex in my opinion.

Regarding inlining of the fallback implementation, I think we do need such a mechanism to handle APIs that fail to inline on the first attempt, given that the current fallback overhead is much heavier and leads to worse performance. And I agree with @merykitty that a more generic solution would be more preferable.

Hi @merykitty , @XiaohongGong , Based on the feedback received, I have modified the patch to not inline on first intrinsic failure, instead I now collect such CallGenerators and only towards the end on incremental inlining I inline expands the fallback implementation on the lines of _string_late_inlines.

This will give opportunity to create constant context for VectorSlice intrinsification and if that fails we inline the fallback implimentation to avoid any costly boxing penalties.

[erifan]

Is it possible to generate _vector_late_inlines candidate again in inline_incrementally_one?

[jatin-bhateja]

Compile::inline_vector_fallback(PhaseIterGVN& igvn) inlines fallback implementation and if fallback is composed of intrinsics which are recorded in _late_inlines it attempts intrinsification. We don't append to _vector_late_inlines while executing fallback generator
https://github.com/jatin-bhateja/jdk/blob/444a35685eed8442b721795824cf0a43c7bb02f8/src/hotspot/share/opto/callGenerator.cpp#L722

[XiaohongGong]

There are regression for these two cases. Do you know the root cause?

Before:
VectorSliceBenchmark.intVectorSliceWithVariableIndex       1024  thrpt    2   6204.489          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex1     1024  thrpt    2   1651.334          ops/ms

After:
VectorSliceBenchmark.intVectorSliceWithVariableIndex       1024  thrpt    2   5626.367          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex1     1024  thrpt    2    960.958          ops/ms

[jatin-bhateja]

There are regression for these two cases. Do you know the root cause?

Before:
VectorSliceBenchmark.intVectorSliceWithVariableIndex       1024  thrpt    2   6204.489          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex1     1024  thrpt    2   1651.334          ops/ms

After:
VectorSliceBenchmark.intVectorSliceWithVariableIndex       1024  thrpt    2   5626.367          ops/ms
VectorSliceBenchmark.longVectorSliceWithConstantIndex1     1024  thrpt    2    960.958          ops/ms

Hi @XiaohongGong I observed that there is quite a lot of run to run variation in these micro even with stock JDK, I collected PMU events and found on AVX512 system there are MISALIGNED vector memory operation in fallback which causes this variation.

[jatin-bhateja]

Hi @XiaohongGong , @merykitty , @erifan let me know if you have other comments.

[erifan]

LGTM, thanks!

[XiaohongGong]

Still LGTM!

[merykitty]

I'm still not convinced with this solution. If the pattern matching method proves itself to be not reliable, then we can proceed with an intrinsics. Otherwise, we risk introduce a change that will eventually become redundant.

[jatin-bhateja]

I'm still not convinced with this solution. If the pattern matching method proves itself to be not reliable, then we can proceed with an intrinsics. Otherwise, we risk introduce a change that will eventually become redundant.

Hi @merykitty , As discussed earlier your suggestions were incorporated in latest version of patch, idea here is not to hold an optimization in anticipation of future optimization. x86 backend changes will still be usable if at later point we decide to use complex pattern matching once TypeVect has constant information. What we have currently is generic handling which can inline any fallback after failed intrinsification attempts. Looking forward to your comments on backend part and any further improvement on existing handling.

Hi @sviswa7 , @iwanowww , May I request you to share your views / comments

[iwanowww]

I briefly looked at the patch.

First of all, I suggest to separate the logic to handle intrinsification failures. It's not specific to the proposed enhancement and will improve handling of intrinsification failures for vector operations.

Speaking of proposed approach, it aligns well with current Vector API implementation practices. I agree it would be nice to automatically detect equivalent IR shapes and transform them accordingly, but if it means hard-coding the shape of sliceTemplate into the compiler, current proposal does look well-justified.

[jatin-bhateja]

Thanks @iwanowww , I agree that approach to inline on intrinsic failure is generic enough and can benefit other vector operations also as it may absorb boxing penalties. For slice and un-slice since the fallback is completely written in vector APIs it will give most benefits and that is the focus of this patch.

Looking forward to your other comments on current implementation.

Thanks @iwanowww , I agree that approach to inline on intrinsic failure is generic enough and can benefit other vector operations also as it may absorb boxing penalties. For slice and un-slice since the fallback is completely written in vector APIs it will give most benefits and that is the focus of this patch.

Looking forward to your other comments on current implementation.