RE: RFR[M]: Adding MD5 Intrinsic on x86-64

[Ludovic Henry]

I've fixed it at [1]. I'm sending an update very soon as soon as I have the 
performance numbers you asked for, and the test suites results on the different 
platforms of interest.

[1] http://cr.openjdk.java.net/~luhenry/8250902/webrev.02/


-----Original Message-----
From: Vladimir Kozlov <vladimir.koz...@oracle.com> 
Sent: Monday, August 3, 2020 4:59 PM
To: Ludovic Henry <luhe...@microsoft.com>; 
hotspot-compiler-...@openjdk.java.net; Vivek Deshpande <viv.d...@gmail.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>
Subject: Re: RFR[M]: Adding MD5 Intrinsic on x86-64

Hmm, with that code reversed I now have failure only on Windows:

V  [jvm.dll+0x43abb7]  report_vm_error+0x117  (debug.cpp:264)
V  [jvm.dll+0x8a222e]  LibraryCallKit::load_field_from_object+0x1ae  
(library_call.cpp:5732)
V  [jvm.dll+0x88c3ea]  LibraryCallKit::get_state_from_digestBase_object+0x3a  
(library_call.cpp:6614)
V  [jvm.dll+0x8909d5]  LibraryCallKit::inline_digestBase_implCompressMB+0x115  
(library_call.cpp:6598)
V  [jvm.dll+0x8908b1]  LibraryCallKit::inline_digestBase_implCompressMB+0x411  
(library_call.cpp:6578)
V  [jvm.dll+0x8a5b2d]  LibraryCallKit::try_to_inline+0x184d  
(library_call.cpp:836)

The bug is in the same code as before - typreo due to renaming. So the code 
should be:

      if (long_state) {
        state = get_long_state_from_digestBase_object(obj);
      } else {
        state = get_state_from_digestBase_object(obj);
      }


BTW, Ludovic, you need to add next change [1] to Graal's test to avoid its 
failure.

Thanks,
Vladimir K

[1]

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java
@@ -423,6 +423,11 @@
                              
"java/math/BigInteger.shiftRightImplWorker([I[IIII)V");
          }

+        if (isJDK16OrHigher()) {
+            add(toBeInvestigated,
+                            "sun/security/provider/MD5.implCompress0([BI)V");
+        }
+
          if (!config.inlineNotify()) {
              add(ignore, "java/lang/Object.notify()V");
          }
@@ -593,6 +598,14 @@
          return JavaVersionUtil.JAVA_SPEC >= 14;
      }

+    private static boolean isJDK15OrHigher() {
+        return JavaVersionUtil.JAVA_SPEC >= 15;
+    }
+
+    private static boolean isJDK16OrHigher() {
+        return JavaVersionUtil.JAVA_SPEC >= 16;
+    }
+
      public interface Refiner {
          void refine(CheckGraalIntrinsics checker);
      }


On 8/3/20 12:18 PM, Vladimir Kozlov wrote:
> I reproduced crash with only MD5 changes on my local linux machine during 
> fastdebug build.
> 
> Next code in inline_digestBase_implCompressMB should be reversed 
> (get_long_*() should be called for long_state):
> 
>     if (long_state) {
>       state = get_state_from_digestBase_object(digestBase_obj);
>     } else {
>       state = get_long_state_from_digestBase_object(digestBase_obj);
>     }
> 
> Vladimir K
> 
> On 8/3/20 11:52 AM, Ludovic Henry wrote:
>>> But it looks like it has more changes (windows_aarch64) then just MD5 
>>> intrinsic.
>>> I will retest again with removed other changes.
>>
>> That looks like a mistake with me learning to use Mercurial, sorry about 
>> that.
>>
>> The only patch you need is `8250902: Implement MD5 Intrinsics on x86`, all 
>> the others are my mistake.
>>