Vector implementation for arraycmp on P8/P9 - #7011
Conversation
|
The new implementation will use The current code looks like this: I wrote a small test to perform
@zl-wang You mentioned you wanted to take a look at this. |
|
that is too much performance difference. i think a few improvement should be pursued:
|
|
Okay, I'll try taking a look at seeing if those instructions can help. |
|
for typical String comparison, if using char (2-byte) and residue iteration is going to be half of the current, i expected no case is going to be 2x slower. |
|
actually, i projected most of the cases would be faster. only 8-byte case could be slower, but almost equal. |
|
The new vector implementation looks like this: One change from before is to avoid a long residue loop when there are fewer than 16 elements left. This is done in two way. First off for arrays smaller than 128, array size is between 16x+8 and 16x+15 where x is an integers, it will fall back to using 8 byte loads. The second is after the vector loop, if there are still 8 or more bytes left there will be one loop of loading with an 8 byte load before doing byte by byte loads. It recently occurred to me that doing both might be too much so it might be possible to remove the first case but I haven't tried this yet. The second change is an attempt at better handling of finding a mismatched byte while in the vector portion of the loop. A path was added to reorder the bytes under little endian to match how they are in the array. The bytes are then copied to GPRs where I got updated performance data. To make the run a bit longer, I increased the number of interations to 1 billion. This table covers the cases where there is a complete match on arrays of different lengths:
Somewhere along the way my implementation got worse for smaller arrays (size 2, 4-7). I used to get better numbers. I think it might have happened when I was making changes for better handing on an array mismatch which was unexpected. I will need to look into this a bit further. For other array sizes up to 32, my new version works better. I also showed the 128 and 1000 length cases to show it works better for larger arrays as well. The following data is for the case where there a mismatched byte in the two arrays being compared. Different array lengths take different paths through the code.
The worst case is the 23 length vector where the mismatch is on the very first byte. This takes the path of going to the vector implementation of finding the mismatched byte. But, this is worse than the byte by byte residue loop. The reason for that is this is the best case for the residue loop as it finds a hit on the very first byte it tries to compare. In general this will also happen for larger arrays as well where it goes to the vector path and the mismatch is on the very first byte. |
|
i felt intuitively it can be improved possibly in the following areas:
i haven't looked at the vectorDiffLabel code carefully yet to see possible improvements. i might come back to this later. |
|
The performance regression mentioned in the earlier post was caused by an unconditional branch to branch around newly added assembly to handle finding mismatched bytes during the vector load section. I rearranged the code to reduce the effects. Using the count register will slightly reduce the number of instructions in the residue loop. I had initially thought it wouldn't due the additional instructions to set up the count registers but it worked out when I tried it. Removed the Added support for the Added support for a 4 byte load section. If there are 4 to 7 bytes left to compare, this path is now taken rather than going straight to the byte loop. Removed looping on the 4 and 8 byte load sections. I realized that these code paths will never loop. The 4 byte load section is only used when there are 7 or fewer bytes left to compare. Similarly, the 8 byte load section is only used when there are 15 of fewer bytes left. As a result, I was able to remove a few extra instructions that were previously used to handle the 4 or 8 byte load sections looping. The remove looping was only recently added so I don't have updated performance numbers for that case yet. Without that last change, the current worst case is when there is a first byte mismatch on an array of size 4, 5, 6 or 7. |
|
The newest change adds an earlier length check against 4 after the vector load section. This will skip past both the 8 byte and 4 byte load sections instead of just skipping over one at a time. The latest implementation looks like this: There is a vector 16 byte load section followed by an 8 byte load, 4 byte load then the residue loop. If a mismatch is found in the vector section, it will jump to I also updated the performance data. The number of interations to 1 billion which is the same as the previous tests. This table covers the cases where there is a complete match on arrays of different lengths:
The vector implementation helps in most case. The worst case seems to be 4% down on a 32 byte array. That path uses two vector loads to handle the entire match so I'm not entirely sure why it seems to be worse. The following data is for the case where there a mismatched byte in the two arrays being compared. Different array lengths take different paths through the code.
The big problem here is being 13% slower when the array is 7 bytes long and the mismatch is on the very first byte. Under the old implementation, it would go straight to the residue loop and get a hit on the first compare. Under the new implementation it attempts to use a 4 byte load, finds a mismatch then takes a path to find the mismatch within the 4 bytes. The next biggest problem is being 5% down on a 19 byte array when there is a mismatch on the first byte. This one needs to do the vector mismatch path to find a mismatch on the first byte. |
|
i don't see it is necessary to have skipLoad8Label2, since the only use of it is after comparing less than 4 already. |
|
That looks right to me. Instead of jumping to |
|
using vnor and vclz instructions to calculate which byte it mismatches seems faster to me. |
|
you should evaluate the alternative no-loop residue code (at most 3 pairs of load and cmp), because, on P8, mtctr is still relatively expensive (5 cycles i remembered). P9 is even worse in that regard. |
|
I don't see a I can also check to see if I can remove the loop on the residue section. |
|
on P9, instead of loading vector of 4 words, you should use loading vector of 16 bytes (such that there is no diff between LE and BE, and no need to rotate the mask result). |
|
Oh, I had that one planned but forgot to mark it down. I will be using that and removing some of the instructions to rearrange the data on Power 9. |
|
I was seeing unusual performance numbers for the test case of comparing 2 identical arrays that were both 32 bytes long. The bad case was about 30% slower. This is case that has good performance: This is the case with bad performance: And this is the code diff between the two runs: The code changes were to the path that handles a mismatched byte after the vector loads. Since the two arrays are identical this path was not expected to be taken. To be certain, I step through the assembly in gdb and confirmed that this path was not taken. @zl-wang Do you have any idea on why this might have happened? |
|
only 32byte-equal case is slower? no other cases became slower? because different code sequence isn't even run in this case, it is indeed hard to understand the slow-down. i can speculate: one instruction less makes branch-prediction hashed to the same entry such that mis-prediction happens more. one way to verify this speculation is to insert a nop in the vnor case, making up that one-less situation. and test again. |
|
I actually just tried adding an extra nop. That makes the performance problem go away for the 32 byte array case. This was the specific case I was looking at but there were also other unusual cases like: |
|
because you are comparing the same logical sequences with different path-lengths with slower cases of one instruction less (everything else is the same), the likely reasons for slowing down: branch prediction related and instruction issue related. your nop test just pinned it to be branch prediction related. I think it probably isn't worth of narrowing down which branch(es) are involved (by inserting the nop at different locations). 19byte case: one vector iteration and mismatching in the first residue iteration etc. i guessed 17byte mismatching on the last byte should be similar. did you see any vnor faster case(s)? especially with the nop inserted. |
|
Changing Since these are all matching arrays they should all be running the exact same instructions but are faster anyways. I haven't tried re-running everything with the nop. But, I did retry the 19 byte array with mismatch an index 17 and it was faster (about the same as going back to |
|
you haven't tried vclzd to replace the two cntlzd and no-loop residue. with vclzd (one instruction less further), the nop might not be needed. run a few times each to see the stable/reliable timing. once we landed on the sweet spot, we can stick to it. |
|
I just tried using |
|
can you show where you inserted 2 nops to recover the performance? |
|
The latest version looks like this: The two They are in unreachable code so they never run themselves. |
|
never-executed nop(s) make the performance better for some cases (some of which have no actual loop). that is a clear indicator branch-prediction aliasing is likely the problem. at the same time, it is unclear whether vclzd (and two nops) or two cntlzd (and one nop) is better, such that i have no preference in this choice. either is fine with me. let's wrap it up with further testing of no residue loop (likely no benefit ... in my mind). |
|
The new changes are as follows: This generates the assembly shown in the previous comment: The new performance data looks like this:
The worst case is still the 32 byte matching array case. But, I think this is just a testing anomoly since this path uses vector loads twice and doesn't have a mismatch in the loaded values. This case should be better than the old system which uses 8 byte loads 4 times instead.
The one bad case here is the 7 length array with a mismatch on the first byte which is a real degredation since the new version is worse at this case than the old version. Note that the baseline value for this case happened to be better than the last time I took the baseline. My latest changes didn't do anything that would effect this particular case so result should be about the same as they were last time. On top of these change, I also made a new change to reduce the number of jumps taken in some cases. I will be writing an update about this shortly. |
a27723c to
e3a5d51
Compare
IBMJimmyk
changed the title
|
I made a change to the code to explicitly check for the datatype of the length child since it can be either Int32 or Int64 and also added a bunch of comments to the code. I retested the code and everything seems to work. @zl-wang Could you help review the code and see if anything else needs to be changed? |
| "lengthNode not int64 or int32. Datatype: %s", node->getDataType().toString()); | ||
|
|
||
| if (isArrayCmpLen && !is64bit) | ||
| if (!is64bit && isLengthNode64bit) |
There was a problem hiding this comment.
from Spencer comment, it seems impossible to have 64bit length node on 32bit platforms. we have no need to handle this combination. plus, it is functionally useless to have that combination. furthermore, it adds complexity to handle register-pair for long on 32bit platforms.
There was a problem hiding this comment.
It is possible to have a 64bit length node on 32bit. I am able to get trees like this on 32bit AIX:
n524n iRegStore gr2 [0xef0972c0] bci=[-1,14,4] rc=0 vc=1494 vn=- li=4 udi=- nc=1
n521n l2i [0xef097200] bci=[-1,10,4] rc=3 vc=1494 vn=- li=4 udi=- nc=1
n520n arraycmplen <arraycmplen>[#244 helper Method] [flags 0x400 0x0 ] () [0xef0971c0] bci=[-1,10,4] rc=1 vc=1494 vn=- li=4 udi=- nc=3 flg=0x20
n509n aiadd (X>=0 internalPtr ) [0xef096f00] bci=[-1,10,4] rc=1 vc=1494 vn=- li=4 udi=- nc=2 flg=0x8100
n546n ==>aRegLoad
n511n iconst 8 (X!=0 X>=0 ) [0xef096f80] bci=[-1,10,4] rc=2 vc=1494 vn=- li=4 udi=- nc=0 flg=0x104
n514n aiadd (X>=0 internalPtr ) [0xef097040] bci=[-1,13,4] rc=1 vc=1494 vn=- li=4 udi=- nc=2 flg=0x8100
n545n ==>aRegLoad
n511n ==>iconst 8
n519n iu2l (highWordZero X>=0 ) [0xef097180] bci=[-1,4,3] rc=1 vc=1494 vn=- li=4 udi=- nc=1 flg=0x4100
n13n ==>iloadi
The length node is a 64bit value in this case.
There was a problem hiding this comment.
but IdiomTransformations will insert an i2l on the length child on 64-bit platforms. (quoted from above)
please confirm it with @Spencer-Comin.
There was a problem hiding this comment.
arraycmplen will always have a length node that is TR::Int64, but it should be impossible for the value to exceed 32 bits on a 32 bit platform (since you can't have an array that occupies 232+ bytes of memory). arraycmp will always have a length node that is TR::Int32 on 32 bit platforms, but on 64 bit platforms it is inconsistent and may be TR::Int32 or TR::Int64.
| bool is64bit = cg->comp()->target().is64Bit(); | ||
| bool isLengthNode64bit = lengthNode->getDataType().isInt64(); | ||
| TR_ASSERT_FATAL_WITH_NODE(lengthNode, lengthNode->getDataType().isInt64() || lengthNode->getDataType().isInt32(), | ||
| "lengthNode not int64 or int32. Datatype: %s", node->getDataType().toString()); |
There was a problem hiding this comment.
provides more precise assert ... need to cover the case: no 64bit length node on 32bit platforms.
48c3fae to
ce90286
Compare
|
This should be good to go now. The length node is now always 64 bit. This is for both @zl-wang Can you take another look when you get a chance? |
| */ | ||
| if (is64bit) | ||
| { | ||
| generateTrg1Src1ImmInstruction(cg, TR::InstOpCode::sradi, node, tempReg, lengthReg, 4); |
There was a problem hiding this comment.
shouldn't logical-shift be used here?
There was a problem hiding this comment.
I can change it, but it shouldn't matter either way. Length values where the highest bit is set shouldn't happen.
|
|
||
| /* If a difference is found in the 16 loaded bytes, jump to load16DiffLabel to figure out where the first mismatched byte is. */ | ||
| generateTrg1Src2Instruction(cg, TR::InstOpCode::vcmpequb_r, node, vec1Reg, vec1Reg, vec2Reg); | ||
| generateConditionalBranchInstruction(cg, TR::InstOpCode::bge, node, load16DiffLabel, cr6Reg); |
There was a problem hiding this comment.
the resulting condition cannot be "greater than" i.e. positive. the more appropriate bc instruction here is beq, i.e. false compare result is set in "equal" bit, although i understood bge here is equivalent to beq in effect.
There was a problem hiding this comment.
vcmpequb. changes the condition register is a special way. If all the bytes in the two vector registers match, the all_true condition is met. This causes the negative bit to be set in CR6. The following instruction wants to jump to load16DiffLabel if a difference in the bytes is found. This is done by testing "greater than or equal" on CR6 since if a difference is found, the negative bit will be clear.
There was a problem hiding this comment.
exactly: CR6 = t || 0b0 || f || 0b0 t & f are true/false from the comparison as you described above. i.e. only lt and eq bits are possibly set. should not be bothered to test gt bit.
There was a problem hiding this comment.
bge only checks 1 bit. It checks if the negative bit is clear. For example:
0x315efe90 00000098 [0xef2f0680] 10000c06 10 vcmpequb. vr0, vr0, vr1
0x315efe94 0000009c [0xef2f06d0] 409800e8 10 bge cr6, Label L0169
0x409800e8 -> bc 4, 24, targetAddr
The first 4 means check if the condition bit is 0.
24 means check (24+32=) bit 56. Bit 56 is the negative flag of CR6.
So it will branch if the negative flag on CR6 is 0 which is what I want.
There was a problem hiding this comment.
ok. that sounds about right.
| generateConditionalBranchInstruction(cg, TR::InstOpCode::beq, node, returnTrueLabel, cr0Reg); | ||
|
|
||
| /* If there are less than 4 bytes left, jump to the byte by byte handling. */ | ||
| generateTrg1Src1ImmInstruction(cg, is64bit ? TR::InstOpCode::cmpi8 : TR::InstOpCode::cmpli4, node, cr6Reg, tempReg, 4); |
There was a problem hiding this comment.
is there any benefit in using diff cmpi instruction here, given the expected tempReg value? it is not that cmpli4 doesn't work in 64bit mode.
There was a problem hiding this comment.
There is no difference. tempReg is never negative at that point or is it ever a large enough positive number for the highest bit to be set. I can change it to be cmpli8 so it looks the same as the 32 bit case.
| if (is64bit) | ||
| { | ||
| /* There are at least 4 bytes left. If there are less than 8 bytes left jump to the 4 byte load section. */ | ||
| generateTrg1Src1ImmInstruction(cg, TR::InstOpCode::cmpi8, node, cr6Reg, tempReg, 8); |
There was a problem hiding this comment.
shouldn't cmpli4 be used here? although we don't support 32bit processors anymore.
There was a problem hiding this comment.
I think cmpli4 can be used there. tempReg technically holds a 64 bit value but it will always be between 4 to 15 at that point.
| /* | ||
| * Under 64bit, there are at least 8 bytes left at this point but no more than 15. | ||
| * Under 32bit, there are at least 4 bytes left at this point but no more than 15. | ||
| */ |
There was a problem hiding this comment.
this comment is misleading ... and you made the handling more complicated than necessary. you can come here only: 1) the length is less than 16; or, 2) you are about to handling the residue; either case is already tested against 4 and 8. no diff to me between 32bit and 64bit.
There was a problem hiding this comment.
Under the 32 bit case, the check against 8 is skipped since the section with 8 byte loads is not run under 32 bit. So under 32 bit there are 4-15 bytes left at this point while under 64 bit there are 8-15
| generateConditionalBranchInstruction(cg, TR::InstOpCode::blt, node, skipLoad4Label, cr6Reg); | ||
|
|
||
| /* There are at least 4 bytes left at this point but no more than 7. */ | ||
| generateLabelInstruction(cg, TR::InstOpCode::label, node, skipLoad8Label); |
There was a problem hiding this comment.
isn't it a problem that this label is only created conditionally?
There was a problem hiding this comment.
The instructions that jump to it are generated under the same condition. As in, the label is only generated under 64 bit but the jumps to the label are also only generated under 64 bit.
| generateTrg1Src1ImmInstruction(cg, TR::InstOpCode::addi2, node, offsetReg, offsetReg, 8); | ||
|
|
||
| /* If there are no more bytes, the arrays match each other. */ | ||
| generateTrg1Src2Instruction(cg, TR::InstOpCode::subf_r, node, tempReg, offsetReg, lengthReg); |
There was a problem hiding this comment.
it seemed sub-optimal to repeat this subf_r instruction over and over again, but i haven't thought through the alternative yet.
There was a problem hiding this comment.
subf. is used to compare how many bytes have been processed to check if we are done and also determine how much is left so we know what section to run next. I can't think of a better way to do this but tell me if you think of something.
| generateTrg1Src1ImmInstruction(cg, TR::InstOpCode::cmpli4, node, cr6Reg, tempReg, 4); | ||
| generateConditionalBranchInstruction(cg, TR::InstOpCode::blt, node, skipLoad4Label, cr6Reg); | ||
|
|
||
| /* Use 4 byte loads to search for a mismatch between the two arrays. */ |
There was a problem hiding this comment.
this lost me. the coming-in condition is more than 4byte but less than 8bytes. you did a 4-byte previously, so that you have less-than-4 to go, don't you?
There was a problem hiding this comment.
This is under the 32 bit case. The 32 bit case doesn't do the 8 byte load section. So in this area is has to do the 4 byte load and comparison up to 3 times. Each times, it checks if there are at least 4 bytes left to process.
ce90286 to
1471f91
Compare
|
I updated the |
| generateConditionalBranchInstruction(cg, TR::InstOpCode::blt, node, skipLoad8Label, cr6Reg); | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
here, you have an issue for choice of laying down optimal sequence and catching the most likely cases fast. for example, length more than 16 is most the likely. doing a single 16 comparison up front, then handling the residual looks better, instead of paying the cost of 2 taken branches (6 cycles in total usually on P, assuming every branch is perfectly predicted). progressively testing for longer lengths might end up paying the highest cost for most situations. have you thought through it?
| generateTrg1Src1ImmInstruction(cg, TR::InstOpCode::addi2, node, offsetReg, offsetReg, 16); | ||
| generateConditionalBranchInstruction(cg, TR::InstOpCode::bdnz, node, load16LoopLabel, cr6Reg); | ||
|
|
||
| /* If there are no more bytes, the arrays match each other. */ |
There was a problem hiding this comment.
reading the code more, i see no reason not to get rid of the up front 4/8 tests.
|
The code is laid out to try and reduce the number of taken branches. This table shows the number of taken branches for each input size:
I think a spent a fair amount of time on this before but that was a few months ago so I'll try giving it another look over. |
|
This change makes performance worse for array sizes of 1-15 by as much as 14%. Array size of 16 benefits from the change by 11%. Roughly as the array gets bigger, the benefit gets smaller. For larger arrays, I tested up to size 1000, both versions perform about the same. |
zl-wang
left a comment
zl-wang
left a comment
There was a problem hiding this comment.
with the expectation that String length is on the short side (24bytes on average in UTF16; expecting 12bytes by and large in compressed), i can accept the pre-testing of 4/8.
|
@dsouzai please review/approve/merge |
|
jenkins build plinux,aix |
|
@IBMJimmyk |
|
Let me take a look at those failures. |
1471f91 to
ea0f4dc
Compare
Modified both arraycmp and arraycmplen evaluators on Power 8 and Power 9 to take advantage of vector instructions. Power 10 already has an existing vector implementation of arraycmp and continues to use it. Added binary encoder tests for the newly used cmpb, vclzd and vclzw instructions. Signed-off-by: jimmyk <jimmyk@ca.ibm.com>
|
I ended up mixing up the expected results for two encoding tests. I fixed the problem so the tests should work now. |
|
jenkins build plinux,aix |
|
plinux failure is due to #6571 |
4 participants
WIP implementation for modifying P8/P9 arraycmp to use vector operations.