[webkit-reviews] review granted: [Bug 184440] DFG AI and clobberize should agree with each other : [Attachment 337592] the patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 184440: DFG AI and clobberize should agree with each other
https://bugs.webkit.org/show_bug.cgi?id=184440

Attachment 337592: the patch

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




--- Comment #5 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 337592
  --> https://bugs.webkit.org/attachment.cgi?id=337592
the patch

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

r=me

> Source/JavaScriptCore/dfg/DFGClobberize.h:979
> +	       read(Butterfly_publicLength);
> +	       read(Butterfly_vectorLength);
> +	       read(ArrayStorageProperties);
> +	       write(ArrayStorageProperties);

For setters, are they always !mayStoreToHole?

> Source/JavaScriptCore/dfg/DFGClobberize.h:1331
> +	   case UntypedUse:
> +	       read(World);
> +	       write(Heap);
> +	       return;

Seems like this would be a good test to add since we don't have one.

> Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h:96
> +    // Would have the last executed node clobbered the world had we not
found a way to fold it?

clobbering the world seems like the wrong comment to have here, since doing a
transition doesn't necessarily entail clobbering the world, but would return
true here. Maybe make the comment say something that allows for both clobbering
the world and transitions?