[webkit-reviews] review granted: [Bug 216214] DFG ASSERTION FAILED: Value not defined in FTLLowerDFGToB3.cpp : [Attachment 408756] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 15 13:29:38 PDT 2020

Saam Barati <sbarati at apple.com> has granted Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 216214: DFG ASSERTION FAILED: Value not defined in FTLLowerDFGToB3.cpp

Attachment 408756: Patch


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

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


> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:209
> +    Allocation& mergeStructures(const Allocation& other)

nit: I think this is only called in one places. I kinda think we should just
inline it into the call site?

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:211
> +	   ASSERT(hasStructures() ||

you should assert that both are empty at the same time, right?

Also, shouldn't structuresForMaterialization be a superset of structures? Maybe
assert that below

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:213
> +	   m_structures.filter(other.structures());
> +	  

can you comment on the nature of these, either here or maybe at their
declaration point in this class, since it's pretty subtle.

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:330
> +	   if (!m_structuresForMaterialization.isEmpty())
> +	      
out.print(inContext(m_structuresForMaterialization.toStructureSet(), context));

can you also print m_structures? And perhaps add some naming to them so we can

> JSTests/stress/allocation-sinking-changing-structures.js:1
> +//@

remove or add something here

More information about the webkit-reviews mailing list