[webkit-reviews] review denied: [Bug 44329] SH4 JIT SUPPORT : [Attachment 87170] YARR for SH4

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied thouraya <thouraya.andolsi at st.com>'s
request for review:
Bug 44329: SH4 JIT SUPPORT
https://bugs.webkit.org/show_bug.cgi?id=44329

Attachment 87170: YARR for SH4
https://bugs.webkit.org/attachment.cgi?id=87170&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87170&action=review

r-, but only due to the putIntegral issues

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:-131
> -	   template<typename IntegralType>
> -	   void putIntegral(IntegralType value)
> -	   {
> -	       if (m_size > m_capacity - sizeof(IntegralType))
> -		   grow();
> -	       putIntegralUnchecked(value);
> -	   }
> -
> -	   template<typename IntegralType>
> -	   void putIntegralUnchecked(IntegralType value)
> -	   {
> -	       *reinterpret_cast_ptr<IntegralType*>(&m_buffer[m_size]) = value;

> -	       m_size += sizeof(IntegralType);
> -	   }
> -

Why is this change here?  This is a cross platform type so adding a new JIT
backend shouldn't  result in functions being removed -- are they only used for
the constant pool backends?

> Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:192
> +    template<typename IntegralType>
> +    void putIntegral(IntegralType value)
> +    {
> +	   if (m_size > m_capacity - sizeof(IntegralType))
> +	       grow();
> +	   putIntegralUnchecked(value);
> +    }
> +
> +    template<typename IntegralType>
> +    void putIntegralUnchecked(IntegralType value)
> +    {
> +	   *reinterpret_cast_ptr<IntegralType*>(&m_buffer[m_size]) = value;
> +	   m_size += sizeof(IntegralType);
> +    }

I think these should stay in the root assembler buffer so we don't have
unnecessary changes in this patch

> Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:205
> +    void putIntegral(TwoShorts value)
> +    {
> +	   if (m_size > m_capacity - sizeof(TwoShorts))
> +	       grow();
> +	   putIntegralUnchecked(value);
> +    }
> +
> +    void putIntegralUnchecked(TwoShorts value)
> +    {
> +	   putIntegralUnchecked(value.high);
> +	   putIntegralUnchecked(value.low);
> +    }

I have endian concerns here