[webkit-reviews] review granted: [Bug 123195] Make JSCells have 32-bit Structure pointers : [Attachment 225174] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 123195: Make JSCells have 32-bit Structure pointers
https://bugs.webkit.org/show_bug.cgi?id=123195

Attachment 225174: Patch
https://bugs.webkit.org/attachment.cgi?id=225174&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225174&action=review


LGTM, but make those changes.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2228
> +    ALWAYS_INLINE Jump branchPtrWithPatch(RelationalCondition cond, Address
left, DataLabel32& dataLabel, TrustedImm32 initialRightValue = TrustedImm32(0))

> +    {
> +	   dataLabel = DataLabel32(this);
> +	   moveWithPatch(initialRightValue,
getCachedDataTempRegisterIDAndInvalidate());
> +	   return branch32(cond, left, dataTempRegister);
> +    }

Rename to branch32WithPatch

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4019
> -	   m_out.storePtr(structure, result, m_heaps.JSCell_structure);
> +	   LValue structureID = m_out.load32(structure,
m_heaps.Structure_structureID);
> +	   LValue indexingType = m_out.load8(structure,
m_heaps.Structure_indexingType);
> +	   LValue typeInfoType = m_out.load8(structure,
m_heaps.Structure_typeInfoType);
> +	   LValue typeInfoFlags = m_out.load8(structure,
m_heaps.Structure_typeInfoFlags);
> +
> +	   m_out.store32(structureID, result, m_heaps.JSCell_structureID);
> +	   m_out.store8(indexingType, result, m_heaps.JSCell_indexingType);
> +	   m_out.store8(typeInfoType, result, m_heaps.JSCell_typeInfoType);
> +	   m_out.store8(typeInfoFlags, result, m_heaps.JSCell_typeInfoFlags);
> +	   m_out.store8(m_out.constInt8(0), result, m_heaps.JSCell_gcData);

The common case is that we're passing a constant structure.  Factor this in
such a way that if structure is a constant, this doesn't emit loads.

> Source/JavaScriptCore/jit/UnusedPointer.h:35
> +#if USE(JSVALUE64)
> +static const uintptr_t unusedPointer = 0x0;
> +#else
>  static const uintptr_t unusedPointer = 0xd1e7beef;
> +#endif

Can we arrange to keep diet beef as the unused pointer?

> Source/JavaScriptCore/runtime/JSCellInlines.h:95
> +inline IndexingType JSCell::indexingType() const
> +{
> +    return (*reinterpret_cast<const uint64_t*>(this) & 0x000000ff00000000)
>> 32;
>  }

Web template framework?

> Source/JavaScriptCore/runtime/Structure.h:510
> +    // These need to be properly aligned at the beginning of the 'Structure'

> +    // part of the object.
> +    StructureID m_structureID;
> +    IndexingType m_indexingType;
> +    TypeInfo m_typeInfo;

Unionify.