8261022: Fix incorrect result of Math.abs() with char type #2419
Conversation
Math.abs() with char type may return incorrect result after C2 superword
optimization. It can be reproduced by below Java code and commands.
public class Bug {
private static int SIZE = 60000;
private static char[] a = new char[SIZE];
private static char[] b = new char[SIZE];
public static void main(String[] args) {
for (int i = 0; i < SIZE; i++) {
a[i] = b[i] = (char) i;
}
for (int i = 0; i < SIZE; i++) {
a[i] = (char) Math.abs(a[i]);
}
for (int i = 0; i < SIZE; i++) {
if (a[i] != b[i]) {
throw new RuntimeException("Broken!");
}
}
System.out.println("OK");
}
}
// $ java -Xint Bug
// OK
// $ java -Xcomp -XX:-TieredCompilation Bug
// Exception in thread "main" java.lang.RuntimeException: Broken!
// at Bug.main(Bug.java:15)
In Java, 'char' is a 16-bit unsigned integer type and the abs() method
should always return the value of its input. But with C2 vectorization,
the sign bit of the 16-bit value is cleared because it's regarded as a
signed value.
Root cause is that we get an imprecise vector element type for AbsINode
from SuperWord::compute_vector_element_type(). In any Java arithmetic
operation, operands of small integer types (boolean, byte, char & short)
should be promoted to int first. As vector elements of small types don't
have upper bits of int, for RShiftI or AbsI operations, the compiler has
to know the precise signedness info of the 1st operand. These operations
shouldn't be vectorized if the signedness info is imprecise.
In code SuperWord::compute_vector_element_type(), we have some special
handling for right shift. It limited the vectorization of small integer
right shift to operations only after loads. The reason is that in the C2
compiler, only LoadNode has precise signedness info of its value. When
JDK-8222074 enabled abs vectorization, it didn't involve AbsI operation
into the special handling and thus introduced this bug. This patch just
does the fix at this point.
Tested hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1,
no new failure is found. Also created a new jtreg with this fix.
|
👋 Welcome back pli! A progress list of the required criteria for merging this PR into |
Webrevs
|
| // Widen type to Int to avoid creation of right shift vector | ||
| // (align + data_size(s1) check in stmts_can_pack() will fail). | ||
| // Note, left shifts work regardless type. | ||
| // Widen type to int to avoid the creation of vector nodes. Note |
There was a problem hiding this comment.
in->Opcode() can be replaced by op
|
@pfustc 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: 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 4 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
openjdk
bot
added
integrated
|
@pfustc Since your change was applied there have been 23 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 7a2db85. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Math.abs() with char type may return incorrect result after C2 superword optimization. It can be reproduced by below Java code and commands.
In Java, 'char' is a 16-bit unsigned integer type and the abs() method should always return the value of its input. But with C2 vectorization, the sign bit of the 16-bit value is cleared because it's regarded as a signed value.
Root cause is that we get an imprecise vector element type for AbsINode from SuperWord::compute_vector_element_type(). In any Java arithmetic operation, operands of small integer types (boolean, byte, char & short) should be promoted to int first. As vector elements of small types don't have upper bits of int, for RShiftI or AbsI operations, the compiler has to know the precise signedness info of the 1st operand. These operations shouldn't be vectorized if the signedness info is imprecise.
In code SuperWord::compute_vector_element_type(), we have some special handling for right shift. It limited the vectorization of small integer right shift to operations only after loads. The reason is that in the C2 compiler, only LoadNode has precise signedness info of its value. When JDK-8222074 enabled abs vectorization, it didn't involve AbsI operation into the special handling and thus introduced this bug. This patch just does the fix at this point.
Tested hotspot::hotspot_all_no_apps, jdk::jdk_core and langtools::tier1, no new failure is found. Also created a new jtreg with this fix.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2419/head:pull/2419$ git checkout pull/2419