Accelerate StringCoding.hasNegatives for JDK 11, 17, StringCoding.countPositives for JDK 21+ on x86

[dylanjtuttle]

This PR accelerates intrinsic candidates StringCoding.hasNegatives and StringCoding.countPositives on x86, the former on JDK 9-18 and the latter on JDK 19+.

This PR is incremental in a few ways.

• The acceleration currently only delivers the desired performance boost for arrays of up to 31 elements. For arrays of 32 elements and longer, the acceleration still outperforms default OpenJ9, but can no longer keep up with HotSpot. For that, I will need to take advantage of larger SIMD instructions.
• I have discovered a strange performance anomaly in OpenJ9 that causes it to perform significantly faster than expected for hasNegatives for arrays of 0-8 elements on JDK 19+. It performs so well for these short arrays that implementing my 'acceleration' would actually cause a performance regression there. While this anomaly is investigated, I will not be accelerating hasNegatives on JDK 19+.

In the interest of taking advantage of the performance boost as it currently stands for the 0.51 release, this PR will deliver these changes in their incremental state, with plans for another PR or two down the road to close the aforementioned gaps.

[dylanjtuttle]

Paging @vijaysun-omr, @0xdaryl for review and @r30shah, @dchopra001 as requested for comparison to acceleration on Z

[vijaysun-omr]

Looks fine to me from a quick review. I'll probably defer to @hzongaro for the review since he has much more direct awareness of this work than I do, and can easily review the inliner parts as well (that I skimmed over too).

[dylanjtuttle]

It appears that the crash is due to a recent OMR commit and is therefore unrelated to my changes. I can build successfully with the openj9 branch of the openj9-omr repo, and can now confirm all of the performance testing checks out.

[BradleyWood]

I think you can simplify this code like the following, no branching required.

xor   resultReg, resultReg  ; Set result to 0
test  chunk, mask
setne resultReg             ; Set result to 1, if not eq

[BradleyWood]

For vector registers you need to call cg->allocateRegister(TR_VRF);, otherwise register assignment, register spilling may not work right.

[BradleyWood]

Use more descriptive naming i -> indexReg, ba -> bufReg, etc.

[BradleyWood]

Why do you need ecx here? Its not used, and as far as I can tell, non of these instructions implicitly use it.Why do you need ecx here? Its not used, and as far as I can tell, non of these instructions implicitly use it.

[BradleyWood]

Why do you need ecx here? Its not used, and as far as I can tell, non of these instructions implicitly use it.Why do you need ecx here? Its not used, and as far as I can tell, non of these instructions implicitly use it.

[BradleyWood]

at least one of the sign bits is set

Don't you need to keep track of the index of last positive element? This function should find the number of leading positive elements so the location of that negative matters. You cannot return (i - off) as you do in the residue processing because you don't know where in the vector the first negative is.

[dylanjtuttle]

The key here was pointed out by Henry early on when I was first trying to figure out how to accelerate countPositives:

    /**
     * Count the number of leading positive bytes in the range.
     *
     * @implSpec the implementation must return len if there are no negative
     *   bytes in the range. If there are negative bytes, the implementation must return
     *   a value that is less than or equal to the index of the first negative byte
     *   in the range.
     */

countPositives only needs to return a value less than or equal to the index of the first negative byte. There are a few places in String.java that call countPositives and then do the work of finding the exact index themselves. I suppose the reason it was designed this way is to take advantage of small time saves in situations where you don't care about the exact index (like when being called by hasNegatives).

[BradleyWood]

fix indentation

[BradleyWood]

You don't use any AVX instructions, as far as I cant tell the highest req you have is SSE 4.1. However, this can be done with just SSE 2 if you use pcmpgtb.

[BradleyWood]

A couple of things with the main loop. You don't need to load this mask to track sign bits. You can compare each byte to 0 ( is 0 > arr[i] ) using pcmpgtb instruction. pcmpgtb produces at bit mask which can be converted into an integer with PMOVMSKB. If the mask is zero, the all vector elements are positive and you continue the loop. Otherwise, you can find the index of the first negative instruction using tzcnt (use bsf if bmi1 not available) instruction.

Here is some rough pseudo-code:

index = off

while (index < loop_limit)
    movdqu  data_xmm, [base_ptr + index]; Load 16 bytes from arr[i]
    pxor tmp_xmm, tmp_xmm               ; create zero vector
    pcmpgtb tmp_xmm, data_xmm           ; Compare: 0 > arr[i] -> 0xFF if negative, 0x00 otherwise
    pmovmskb neg_mask, tmp_xmm          ; Extract bitmask of negative values

    test    neg_mask, neg_mask          ; Check if any negative values exist
    jnz     .foundNegative              ; If mask is nonzero, handle first negative
    add     index, 16                   ; all positive
                                        ; loop
.foundNegative
    count = tzcount(neg_mask) + index - off
    jmp .done

.residue
    ...
.done
    ...

Secondly, you do not need to track bytes_left, its like having two loop index counters. Before entering the loop, calculate the loop limit as (len + off) & ~15. You only need to align to vector length. if (i < loop_limit) then execute the loop.

[BradleyWood]

Excluding this call for JDK < 19 is OK, but why not do the same for the method declaration?

[0xdaryl]

I will let you process and action all of Brad's comments before reviewing this thoroughly, but one thing I will mention right away are my strong opinions on the use of the auto keyword and its impact on future code understanding for anyone but the author. I may be known to let one or two slip in on occasion, but since you have many (12) in succession I'll ask that you specify the data type for each (they look to be either TR::Node *'s or TR::Register *'s in your case.

I'll echo Brad's comments about variable naming as well. It is easier to read the code later if the variables that contain nodes are suffixed with Node and registers are suffixed with Reg.