[webkit-reviews] review denied: [Bug 211540] GetArrayLength should be "blessed" during Fixup phase in the DFG : [Attachment 398872] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 211540: GetArrayLength should be "blessed" during Fixup phase in the DFG
https://bugs.webkit.org/show_bug.cgi?id=211540

Attachment 398872: Patch

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




--- Comment #12 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 398872
  --> https://bugs.webkit.org/attachment.cgi?id=398872
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +	   to be configured during Fixup, then right now we will fail for

still not obvious here what you mean by "fail"

> Source/JavaScriptCore/ChangeLog:26
> +	   previously pass it's own speculation for the speculation of the
index, which logically

it's => its

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:433
> +    if (isSomeTypedArrayView()) {
> +	   if (type() == Array::AnyTypedArray)
> +	       return value.isType(SpecTypedArrayView);
> +	   return
value.isType(speculationFromTypedArrayType(typedArrayType()));
> +    }

can you construct a test where your old code would've been broken?

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2031
> +		   arrayMode = arrayMode.withType(Array::ForceExit);

assert we can exit here.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2032
> +	       node->setArrayMode(arrayMode);

assert "isSpecific() here?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:901
> +	       speculationCheck(BadType, JSValueSource::unboxedCell(baseReg),
0, m_jit.branchIfNotType(baseReg, JSType(FirstTypedArrayType),
JSType(LastTypedArrayTypeExcludingDataView)));

nullptr, not zero

> Source/JavaScriptCore/jit/AssemblyHelpers.h:991
> +	   if (last && *last != first) {
> +	       ASSERT(*last > first);
> +	       GPRReg scratch = scratchRegister();
> +	       load8(Address(cellGPR, JSCell::typeInfoTypeOffset()), scratch);
> +	       sub32(TrustedImm32(first), scratch);
> +	       return branch32(Below, scratch, TrustedImm32(*last - first));
> +	   }

this is unused. I'd revert your change here

> Source/JavaScriptCore/jit/AssemblyHelpers.h:996
> +    Jump branchIfNotType(GPRReg cellGPR, JSType first, Optional<JSType> last
= WTF::nullopt)

You should comment that first/last is inclusive

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1003
> +	       return branch32(AboveOrEqual, scratch, TrustedImm32(*last -
first));

this is wrong, since "last" here is inclusive, you're going to say you're not
the 'last' value AFAICT