[webkit-reviews] review denied: [Bug 172957] We should not claim that SpecEmpty is filtered out of cell checks on 64 bit platforms : [Attachment 312361] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Saam Barati <sbarati at apple.com>'s
request for review:
Bug 172957: We should not claim that SpecEmpty is filtered out of cell checks
on 64 bit platforms
https://bugs.webkit.org/show_bug.cgi?id=172957

Attachment 312361: patch

https://bugs.webkit.org/attachment.cgi?id=312361&action=review




--- Comment #19 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 312361
  --> https://bugs.webkit.org/attachment.cgi?id=312361
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312361&action=review

I think it's correct, but I think there are two style goofs:

- The cellCheckTypePassthrough is not specific to the DFG - it's just how cell
checks work everywhere, including LLInt, Baseline, and JSValue.  I think it
should be defined in SpeculatedType.h and follow the same naming convention as
everything else (so SpecCellBlahBlah).

- There seem to be places where you don't use this type constant and instead
you introduce #if's.  You should probably use the constant you introduced.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:63
> +// When doing a cell check, this is the type that passes through
> +static constexpr SpeculatedType cellCheckTypePassthrough = is64Bit() ?
SpecCellOrEmpty : SpecCell;
> +

I think this should be SpecCellCheck, defined in SpeculatedType.h.  Even
JSValue's isCell() has this issue, so it's not DFG-specific.

I'd call it SpecCellCheck, but I'm also OK with SpecCellChekTypePassthrough, or
whatever.  Shorter names are better IMO, but I don't care too much.  I care
more about this not being DFG-specific, and being named SpecBlah like all the
other SpeculatedType constants.

> Source/JavaScriptCore/dfg/DFGUseKind.h:120
> +#if USE(JSVALUE64)
> +	   return SpecCellOrEmpty;
> +#else
> +	   return SpecCell;
> +#endif

Shouldn't this use SpecCellCheck, or whatever it ends up being called?

> Source/JavaScriptCore/dfg/DFGUseKind.h:128
> +#if USE(JSVALUE64)
> +	   return SpecCellOrEmpty | SpecOther;
> +#else
>	   return SpecCell | SpecOther;
> +#endif

Ditto.

> Source/JavaScriptCore/dfg/DFGUseKind.h:171
> +#if USE(JSVALUE64)
> +	   return ~SpecCellOrEmpty;
> +#else
>	   return ~SpecCell;
> +#endif

Ditto.

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13447
>>> +	     FTL_TYPE_CHECK(jsValueValue(cell), edge, SpecSymbol,
isNotSymbol(cell));
>> 
>> We can go either way for all these various speculateX functions. They all do
lowCell prior to being called, so we're guaranteed that ~SpecCell is not in the
set. However, we could leave this code as is (this is how the DFG writes many
of its speculation checks). This would leave these functions open to folks
doing their own manual checks that haven't updated AI state.
> 
> Alternatively, we could assert against the current state of things. So if
folks started using this in the way described above, they'd hit the assert, and
be forced to write the more general version.

I guess speculateSymbol was modeled after speculateString.