[webkit-qt] Release assert in JIT on ARM

[Konstantin Tokarev]

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