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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=216214

Attachment 408756: Patch

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




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

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

r=me

> 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() ||
other.structuresForMaterialization().isEmpty());

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());
> +	  
m_structuresForMaterialization.merge(other.structuresForMaterialization());

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
distinguish.

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

remove or add something here