8253191: C2: Masked byte comparisons with large masks produce wrong result on x86

Reviewed-by: thartmann

Author: Vladimir Ivanov

src/hotspot/cpu/x86/x86_64.ad

@@ -2909,6 +2909,16 @@ operand immI2()
   interface(CONST_INTER);
 %}
 
+operand immU7()
+%{
+  predicate((0 <= n->get_int()) && (n->get_int() <= 0x7F));
+  match(ConI);
+
+  op_cost(5);
+  format %{ %}
+  interface(CONST_INTER);
+%}
+
 operand immI8()
 %{
   predicate((-0x80 <= n->get_int()) && (n->get_int() < 0x80));
@@ -11743,7 +11753,7 @@ instruct compB_mem_imm(rFlagsReg cr, memory mem, immI8 imm)
   ins_pipe(ialu_cr_reg_mem);
 %}
 
-instruct testUB_mem_imm(rFlagsReg cr, memory mem, immU8 imm, immI0 zero)
+instruct testUB_mem_imm(rFlagsReg cr, memory mem, immU7 imm, immI0 zero)
 %{
   match(Set cr (CmpI (AndI (LoadUB mem) imm) zero));
 

test/hotspot/jtreg/compiler/c2/TestUnsignedByteCompare.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, Red Hat Inc. All rights reserved.
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,72 +23,67 @@
 
 /*
  * @test
- * @bug 8204479
- * @summary Bitwise AND on byte value sometimes produces wrong result
+ * @bug 8204479 8253191
  *
- * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:-TieredCompilation
- *      -XX:-UseOnStackReplacement -XX:-BackgroundCompilation -Xcomp -XX:-Inline
- *      compiler.c2.TestUnsignedByteCompare
+ * @library /test/lib
+ * @modules java.base/jdk.internal.vm.annotation
+ *
+ * @run main/bootclasspath/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:-TieredCompilation compiler.c2.TestUnsignedByteCompare
  */
-
 package compiler.c2;
 
+import java.lang.invoke.*;
+import jdk.internal.vm.annotation.DontInline;
+import jdk.test.lib.Asserts;
+
 public class TestUnsignedByteCompare {
 
-    static int p, n;
+    @DontInline static boolean testByteGT0(byte[] val) { return (val[0] & mask()) >  0; }
+    @DontInline static boolean testByteGE0(byte[] val) { return (val[0] & mask()) >= 0; }
+    @DontInline static boolean testByteEQ0(byte[] val) { return (val[0] & mask()) == 0; }
+    @DontInline static boolean testByteNE0(byte[] val) { return (val[0] & mask()) != 0; }
+    @DontInline static boolean testByteLE0(byte[] val) { return (val[0] & mask()) <= 0; }
+    @DontInline static boolean testByteLT0(byte[] val) { return (val[0] & mask()) <  0; }
 
-    static void report(byte[] ba, int i, boolean failed) {
-        // Enable for debugging:
-        // System.out.println((failed ? "Failed" : "Passed") + " with: " + ba[i] + " at " + i);
+    static void testValue(byte b) {
+        byte[] bs = new byte[] { b };
+        Asserts.assertEquals(((b & mask()) >  0), testByteGT0(bs), errorMessage(b, "GT0"));
+        Asserts.assertEquals(((b & mask()) >= 0), testByteGE0(bs), errorMessage(b, "GE0"));
+        Asserts.assertEquals(((b & mask()) == 0), testByteEQ0(bs), errorMessage(b, "EQ0"));
+        Asserts.assertEquals(((b & mask()) != 0), testByteNE0(bs), errorMessage(b, "NE0"));
+        Asserts.assertEquals(((b & mask()) <= 0), testByteLE0(bs), errorMessage(b, "LE0"));
+        Asserts.assertEquals(((b & mask()) <  0), testByteLT0(bs), errorMessage(b, "LT0"));
     }
 
-    static void m1(byte[] ba) {
-        for (int i = 0; i < ba.length; i++) {
-            if ((ba[i] & 0xFF) < 0x10) {
-               p++;
-               report(ba, i, true);
-            } else {
-               n++;
-               report(ba, i, false);
+    public static void main(String[] args) {
+        for (int mask = 0; mask <= 0xFF; mask++) {
+            setMask(mask);
+            for (int i = 0; i < 20_000; i++) {
+                testValue((byte) i);
             }
         }
+        System.out.println("TEST PASSED");
     }
 
-    static void m2(byte[] ba) {
-        for (int i = 0; i < ba.length; i++) {
-            if (((ba[i] & 0xFF) & 0x80) < 0) {
-               p++;
-               report(ba, i, true);
-            } else {
-               n++;
-               report(ba, i, false);
-            }
-        }
+    static String errorMessage(byte b, String type) {
+        return String.format("%s: val=0x%x mask=0x%x", type, b, mask());
     }
 
-    static public void main(String[] args) {
-        final int tries = 1_000;
-        final int count = 1_000;
+    // Mutable mask as a compile-time constant.
 
-        byte[] ba = new byte[count];
+    private static final CallSite     MASK_CS = new MutableCallSite(MethodType.methodType(int.class));
+    private static final MethodHandle MASK_MH = MASK_CS.dynamicInvoker();
 
-        for (int i = 0; i < count; i++) {
-            int v = -(i % 126 + 1);
-            ba[i] = (byte)v;
-        }
-
-        for (int t = 0; t < tries; t++) {
-            m1(ba);
-            if (p != 0) {
-                throw new IllegalStateException("m1 error: p = " + p + ", n = " + n);
-            }
+    static int mask() {
+        try {
+            return (int) MASK_MH.invokeExact();
+        } catch (Throwable t) {
+            throw new InternalError(t); // should NOT happen
         }
+    }
 
-        for (int t = 0; t < tries; t++) {
-            m2(ba);
-            if (p != 0) {
-                throw new IllegalStateException("m2 error: p = " + p + ", n = " + n);
-            }
-        }
+    static void setMask(int mask) {
+        MethodHandle constant = MethodHandles.constant(int.class, mask);
+        MASK_CS.setTarget(constant);
     }
 }