(0.14) Fix and re-enable vectorized String.indexOf on 64-bit Power #5287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Due to the way that sign- and zero-extension of byte and char values works on Power, the target character passed into the intrinsicIndexOf helpers was sometimes sign-extended and sometimes not. However, the evaluators responsible for generating the vectorized code for these helpers was assuming that these values would always be zero-extended. This was specifically causing problems when searching for bytes with negative values, as they were sometimes sign-extended. This has been fixed by making the evaluators always mask off the upper bits before performing the initial scalar comparison, which is the only part of the code which is making this assumption. Signed-off-by: Ben Thomas <ben@benthomas.ca>
Previously, the code for inlining intrinsicIndexOf on Power was making the assumption that the upper 32 bits of the registers containing the array indices were well-defined (i.e. either zero extended or sign extended). As it turns out, many 32-bit operations on Power can cause the upper 32 bits of a register to be undefined. By making use of those bits, we were causing undefined behaviour and, in some cases, would read far beyond the end of the array, resulting in a segfault. This has been corrected and the evaluator responsible for inlining the vectorized versions of these methods will now correctly sign extend the indices before making use of them. Signed-off-by: Ben Thomas <ben@benthomas.ca>
Previously, the vectorized helpers for String.indexOf were disabled on Power due to some difficult to reproduce segfaults that they appeared to be causing. Now that these have (hopefully) been resolved, it should now be safe to re-enable the vectorized versions of the helpers. Note that these have only been re-enabled on 64-bit. These versions were never intended to run on 32-bit and should not have been enabled. Signed-off-by: Ben Thomas <ben@benthomas.ca>
pshipton
approved these changes
Mar 28, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #5211, the fix for vectorized
String.indexOfon 64-bit Power was merged after the 0.14 code split to allow further testing to weed out any possible issues before it was added to 0.14.I've been watching both internal and external tests since the original change was merged and haven't seen any failures that could be attributed to this code being enabled again. Based on that, it should be safe to enable this code for the 0.14 release.