[webkit-qt] Release assert in JIT on ARM

[Konstantin Tokarev]

For the record, I've finally merged this patch into qtwebkit-stable branch (with correction).

25.08.2016, 16:30, "Andrew Webster" <awebster at arcx.com>:
> Konstantin,
>
> The patch uses AnyIntUse, which is not defined. MachineIntUse was renamed to AnyIntUse, see https://bugs.webkit.org/show_bug.cgi?id=156941.
>
> The corresponding git commit is 39e9c984. I guess you would either need to grab that commit as well, or rename AnyIntUse back to MachineIntUse. I renamed back to MachineIntUse for testing and can confirm that it fixes my problem. I have not yet run the JSC test suite.
>
> Thanks a lot Yusuke and Konstantin!
>
> Andrew
> ________________________________________
> From: Konstantin Tokarev <annulen at yandex.ru>
> Sent: Tuesday, August 23, 2016 2:00 PM
> To: Yusuke SUZUKI
> Cc: Andrew Webster; webkit-qt at lists.webkit.org; jsc-dev at lists.webkit.org
> Subject: Re: [webkit-qt] Release assert in JIT on ARM
>
> 23.08.2016, 20:59, "Yusuke SUZUKI" <utatane.tea at gmail.com>:
>>  Fixed. https://trac.webkit.org/changeset/204699
>>
>>  So, I think Konstantin will update the QtWebKitNG for the next Technology Preview.
>>  Once it is done & released, this issue is fixed :)
>
> I've prepared a backport of this patch to qtwebkit-stable branch, but haven't checked it yet:
>
> https://github.com/annulen/webkit/commit/26abba03efc2013b3937a32c815c0111572f225c
>
>>  On Fri, Aug 19, 2016 at 10:34 PM, Yusuke SUZUKI <utatane.tea at gmail.com> wrote:
>>>  Nice catch!
>>>
>>>  I've just filed it in https://bugs.webkit.org/show_bug.cgi?id=161029.
>>>  AnyInt includes int52 representation, that is only allowed in 64bit DFG. (See enableInt52())
>>>
>>>  On Sat, Aug 20, 2016 at 2:49 AM, Konstantin Tokarev <annulen at yandex.ru> wrote:
>>>>  19.08.2016, 20:43, "Konstantin Tokarev" <annulen at yandex.ru>:
>>>>>  19.08.2016, 18:34, "Andrew Webster" <awebster at arcx.com>:
>>>>>>   This may be a question for webkit-dev, but I thought I'd check here first since I'm using qtwebkit-tp3.
>>>>>>
>>>>>>   On an arm 32-bit platform in SpeculativeJIT::speculate, I occasionally hit the default handler which contains a release assert when using the WebInspector:
>>>>>>
>>>>>>   switch (edge.useKind()) {
>>>>>>
>>>>>>   ...
>>>>>>
>>>>>>   default:
>>>>>>       RELEASE_ASSERT_NOT_REACHED();
>>>>>>       break;
>>>>>>   }
>>>>>>
>>>>>>   The value of edge.useKind() causing this is MachineIntUse. The case handler for this value has been ifdef'd out on my platform:
>>>>>>
>>>>>>   #if USE(JSVALUE64)
>>>>>>       case MachineIntUse:
>>>>>>           speculateMachineInt(edge);
>>>>>>           break;
>>>>>>       case DoubleRepMachineIntUse:
>>>>>>           speculateDoubleRepMachineInt(edge);
>>>>>>           break;
>>>>>>   #endif
>>>>>>
>>>>>>   It appears that MachineIntUse is being set in JSC::DFG::FixupPhase::fixupNode when op is ProfileType:
>>>>>>
>>>>>>   if (typeSet->doesTypeConformTo(TypeMachineInt)) {
>>>>>>       if (node->child1()->shouldSpeculateInt32())
>>>>>>           fixEdge<Int32Use>(node->child1());
>>>>>>       else
>>>>>>           fixEdge<MachineIntUse>(node->child1());
>>>>>>       node->remove();
>>>>>>   }
>>>>>>
>>>>>>   I am not at all familiar with this code, but from other usage of MachineIntUse, I would guess that this should not be used except on a 64-bit platform. Given that, I am not sure if
>>>>>>
>>>>>>   1. The typeSet should not conform to TypeMachineInt on 32-bit,
>>>>>>
>>>>>>   2. shouldSpeculateInt32 should always be true on 32-bit,
>>>>>>
>>>>>>   3. Int32Use should always be used on 32-bit, or
>>>>>>
>>>>>>   4. Something else.
>>>>>>
>>>>>>   I currently am going with 3:
>>>>>>
>>>>>>   if (typeSet->doesTypeConformTo(TypeMachineInt)) {
>>>>>>   #if USE(JSVALUE64)
>>>>>>       if (node->child1()->shouldSpeculateInt32())
>>>>>>   #endif
>>>>>>           fixEdge<Int32Use>(node->child1());
>>>>>>   #if USE(JSVALUE64)
>>>>>>       else
>>>>>>           fixEdge<MachineIntUse>(node->child1());
>>>>>>   #endif
>>>>>>
>>>>>>   }
>>>>>>
>>>>>>   This has solved my immediate problem, but due to my lack of understanding, this solution could be quite flawed.
>>>>>>
>>>>>>   Any help is much appreciated.
>>>>>
>>>>>  Hello, thanks for the interest!
>>>>>
>>>>>  I'm by no means a JSC expert, however from quick analysis it seems to me that the correct code would be
>>>>>
>>>>>  #if USE(JSVALUE64)
>>>>>              if (typeSet->doesTypeConformTo(TypeMachineInt)) {
>>>>>                  if (node->child1()->shouldSpeculateInt32())
>>>>>                      fixEdge<Int32Use>(node->child1());
>>>>>                  else
>>>>>                      fixEdge<MachineIntUse>(node->child1());
>>>>>                  node->remove();
>>>>>              }
>>>>>  #else
>>>>>              if (typeSet->doesTypeConformTo(TypeMachineInt) && node->child1()->shouldSpeculateInt32()) {
>>>>>                  fixEdge<Int32Use>(node->child1());
>>>>>                  node->remove();
>>>>>              }
>>>>>  #endif
>>>>>
>>>>>  Anyway, I highly recommend you to:
>>>>>
>>>>>  1. Ask real JSC experts on webkit-dev or jsc-dev
>>>>>  2. Run JSC test suite on target (better debug build as well, as it has much more ASSERTs) before and after such changes
>>>>
>>>>  Sorry, I forgot to add an explanation: AFAIU, MachineInt is Int32 | Int52 and on 32-bit platforms we don't speculate about Int52 because it won't fit in the register anyway, so MachineInt can be only Int32. If we have a MachineInt which is not inferred to be Int32, we cannot do anything fast with it and we follow to the next branch TypeNumber | TypeMachineInt.
>>>>
>>>>  --
>>>>  Regards,
>>>>  Konstantin
>>>>  _______________________________________________
>>>>  webkit-qt mailing list
>>>>  webkit-qt at lists.webkit.org
>>>>  https://lists.webkit.org/mailman/listinfo/webkit-qt
>
> --
> Regards,
> Konstantin

-- 
Regards,
Konstantin