Bug 46796 – Extend constant pool to be able to store 16 bits instructions with a constant

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gabor Loki]

Currently the AssemblerBufferWithConstantPool is only able to store 32 bits instructions with 32 bits constants. We should add 16 bits instruction with 32 bits constant support as well. With this feature the SH4 architecture (and maybe the Thumb-2 JIT) will able to use the constant pool framework.

[Gabor Loki]

Created attachment 69188 [details]
Extend constant pool to be able to store 16 bit instructions with a constant

[thouraya]

Created attachment 76512 [details]
fix unaligned access address for SH4 platform

Hi,

A patch needed to fix unaligned access address for SH4 platform,

To branch around the constant pool we need to instructions,
if we emit them in a single 32 bit instruction, we will get a misaligned address.

Regards.

[WebKit Review Bot]

Attachment 76512 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h', u'JavaScriptCore/assembler/SH4AssemblerBufferWithConstantPool.h']" exit_code: 1
JavaScriptCore/assembler/SH4AssemblerBufferWithConstantPool.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/assembler/SH4AssemblerBufferWithConstantPool.h:47:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/SH4AssemblerBufferWithConstantPool.h:56:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[thouraya]

Created attachment 76547 [details]
fix unaligned access address for SH4 platform

[Gabor Loki]

Comment on attachment 76547 [details]
fix unaligned access address for SH4 platform

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

> JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:278
>          if (useBarrier)
> -            putIntegral(AssemblerType::placeConstantPoolBarrier(m_numConsts * sizeof(uint32_t) + alignPool));
> +            branchAroundConstantPool(m_numConsts * sizeof(uint32_t) + alignPool);

Ohh, why don't you place this extra padding into the Assember's placeConstantPoolBarrier function instead.

For example:
static SH4Word placeConstantPoolBarrier(int offset)
{
+  offset -= sizeof(short);
  ASSERT(((offset >> 1) <=2047) && ((offset >> 1) >= -2048));
  return (0XA000 |(offset >> 1));
}

ARM-JIT has something similar.

[thouraya]

Hi,

In the function placeConstantPoolBarrier, I shoold emit 2 instructions since the branch is a delay slot instruction.

The function is static, so I can't emit directly the branch instruction and just return the opcode of the second instruction.
And I can't emit the 2 instructions as a 32 bit instruction, the address may be misaligned.


Regards.

(In reply to comment #5)
> (From update of attachment 76547 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76547&action=review
> 
> > JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:278
> >          if (useBarrier)
> > -            putIntegral(AssemblerType::placeConstantPoolBarrier(m_numConsts * sizeof(uint32_t) + alignPool));
> > +            branchAroundConstantPool(m_numConsts * sizeof(uint32_t) + alignPool);
> 
> Ohh, why don't you place this extra padding into the Assember's placeConstantPoolBarrier function instead.
> 
> For example:
> static SH4Word placeConstantPoolBarrier(int offset)
> {
> +  offset -= sizeof(short);
>   ASSERT(((offset >> 1) <=2047) && ((offset >> 1) >= -2048));
>   return (0XA000 |(offset >> 1));
> }
> 
> ARM-JIT has something similar.

[thouraya]

Hello,

Did you had a look at my explanation ?

It's not a problem of offset.
I have to store 2 instructions of 16 bits in the buffer.
I can't have an access to m_buffer to store one of the instructions and return the opcode of the second instruction.

If stored as a single instruction of 32 bits, we can have a misaligned address.

Regards,
Thouraya.

[thouraya]

Created attachment 83809 [details]
fix unaligned access address for SH4 platform

[thouraya]

Created attachment 83811 [details]
fix unaligned access address for SH4 platform

[thouraya]

Hi Gabor,

Could you please review the patch ?

Best Regards.
Thouraya.

[Gabor Loki]

Comment on attachment 83811 [details]
fix unaligned access address for SH4 platform

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

> Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:269
> +        // if the code buffer is notA32 bits aligned emit a nop...
> +        if (useBarrier && !AssemblerBuffer::isAligned(4))
> +            AssemblerBuffer::putShort(AssemblerType::padForAlign16);
> +

The issue here is the putShort inserts the padForAlign16 before the barrier instruction. The padForAlign* should be an invalid instruction, and not NOP. So, executing this instruction would cause an illegal instruction error. (Your patch would cause an illegal instruction error if Thumb2 JIT were using the constant pool.)

I suggest you to redesign or extend my patch for your needs. See the following code:
 if (useBarrier)
-   AssemblerBuffer::putInt(AssemblerType::placeConstantPoolBarrier(m_numConsts * sizeof(uint32_t) + alignPool));
+   putIntegral(AssemblerType::placeConstantPoolBarrier(m_numConsts * sizeof(uint32_t) + alignPool));

Here I used putIntegral template function where the AssemblerType::placeConstantPoolBarrier is called for the parameter. You can use this infrastructure for SH4 as well. Create your own AssemblerType::placeConstantPoolBarrier function which returns a structure of two shorts, and implement the putIntegral template specialization which inserts the shorts into the assembler buffer. I think this will help you out.

[thouraya]

Created attachment 84052 [details]
fix unaligned access address

[thouraya]

Hi Gabor,

I added a patch including an implementation of the putIntegral template which inserts  two shorts into the assembler buffer. 

Is it OK?

Regards,
thouraya.

[Gabor Loki]

Comment on attachment 84052 [details]
fix unaligned access address

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

> Source/JavaScriptCore/assembler/AssemblerBuffer.h:46
> +        typedef struct barrier {
> +            short high;
> +            short low;
> +        } barrierType;

Your extension looks OK, but the name of this structure is a little be confusing. Probably TwoShorts or something similar would be better.

Btw, at first I would like to update my patch to TOT and get a reviewer for it. After your changes will be reviewed as well.

Hello,

Thank you Gabor for the answer.
I'll wait for you to update your patch.

Regards,
Thouraya.

[Gabor Loki]

Created attachment 85449 [details]
Extend constant pool to be able to store 16 bit instructions with a constant

I have updated my patch which supports 16 bit stores on the pool.

[Csaba Osztrogonác]

Comment on attachment 85449 [details]
Extend constant pool to be able to store 16 bit instructions with a constant

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

LGTM, r=me with a small style fix.

> Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:251
> +        if (isReusable)
> +            for (int i = 0; i < m_numConsts; ++i) {
> +                if (m_mask[i] == ReusableConst && m_pool[i] == constant) {
> +                    putIntegral(static_cast<IntegralType>(AssemblerType::patchConstantPoolLoad(insn, i)));
> +                    correctDeltas(sizeof(IntegralType));
> +                    return;
> +                }
> +            }

Please add { ... }

[Gabor Loki]

Comment on attachment 85449 [details]
Extend constant pool to be able to store 16 bit instructions with a constant

landed in r81577.

[thouraya]

Created attachment 86911 [details]
fix unaligned data access 

Hello,

I have updated my patch to the TOT.

Regards,
Thouraya.

[Gabor Loki]

Comment on attachment 86911 [details]
fix unaligned data access 

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

> Source/JavaScriptCore/ChangeLog:12
> +        (JSC::AssemblerBuffer::putIntegral):
> +        (JSC::AssemblerBuffer::putIntegralUnchecked):

Hmm, I think they should be in AssemblerBufferWithConstantPool instead. They are rather related to AssemblerBufferWithConstantPool class than AssemblerBuffer.
Others look fine, but how do you want to use this on bug 44329? The 'patchContantPool' function does not return with TwoShorts type.

[thouraya]

Hello,

(In reply to comment #20)
> (From update of attachment 86911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86911&action=review
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        (JSC::AssemblerBuffer::putIntegral):
> > +        (JSC::AssemblerBuffer::putIntegralUnchecked):
> 
> Hmm, I think they should be in AssemblerBufferWithConstantPool instead. They are rather related to AssemblerBufferWithConstantPool class than AssemblerBuffer.

So, all putIntegral and putIntegralUnchecked implementations shoold be moved to AssemblerBufferWithConstantPool .h file.
  
> Others look fine, but how do you want to use this on bug 44329? The 'patchContantPool' function does not return with TwoShorts type.

placeConstantPoolBarrier(int offset) is returning TwoShorts.

[Gabor Loki]

> So, all putIntegral and putIntegralUnchecked implementations shoold be moved to AssemblerBufferWithConstantPool .h file.

Yes, please!

> placeConstantPoolBarrier(int offset) is returning TwoShorts.

Ohh, I missed that. Well, in this case please merge your patch with the "YARR for SH4" one in bug 44329.

[thouraya]

Comment on attachment 86911 [details]
fix unaligned data access 

Merged with YARR patch in Bug 44329.

[Gabor Loki]

fixed r81577.