SH4 JIT SUPPORT

[thouraya]

Created attachment 64941 [details]
SH4 build system support

Hello,


We have ported in STMicroelectronics the squirrelfish javascript engine to SH4 processor.

We would like to contribute the SH4 JIT support for JavaScriptCore.

Here attached a first patch generated for the revision 65071 containing : 
 - some changes in the build system to support sh4 GDK/DIRECTFB back-end.
 - some changes in build-jsc to generate the jsc executable for gtk.
 - some changes in the GtkLauncher to use proxy.

Next time I will provide a patch for JIT and another one for plugin support for GDK/DIRECTFB.


Feedbacks are welcome.

Best regards.
Thouraya.

[thouraya]

Hello,

Can someone give me the steps to follow to contribute the SH4 JIT support for JavaScriptCore ?

Best regards,
Thouraya.

[Patrick R. Gansterer]

I'm not a reviewer but this patch is a clear r-:
1) Missing ChangeLog
2) You change many independent things in one patch
3) You change introduce stuff noone uses. E.g.:
> +#if defined(__sh__)
> +#define WTF_CPU_SH 1
> +#endif
> +
There is no check for CPU(SH) in the code.
4) You cange wrong things. E.g.:
> -#if !(defined(USE_SYSTEM_MALLOC) && USE_SYSTEM_MALLOC) && defined(NDEBUG)
> +#if !(defined(USE_SYSTEM_MALLOC) && USE_SYSTEM_MALLOC) && defined(NDEBUG) || CPU(SH4)
>  #define FORCE_SYSTEM_MALLOC 0
>  #else
>  #define FORCE_SYSTEM_MALLOC 1
This is the wrong place for a CPU(SH4)

See http://webkit.org/coding/contributing.html for more information.

[Gabor Loki]

> Can someone give me the steps to follow to contribute the SH4 JIT support for JavaScriptCore ?

There is a nice example in bug 30144 how to contribute a new JIT port to WebKit.

1.- introduce the basic functionality of the SH4 JIT for the YARR module.
2.- extend the SH4Assembler and MacroAssemblerSH4 file to support the remaining part of JIT
3.- enable the SH4 JIT by default

[thouraya]

Created attachment 67675 [details]
SH4 JIT for YARR

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
ptCore/assembler/SH4Assembler.h:1351:  movw_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1359:  movw_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1367:  movw_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1382:  movw_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1389:  movl_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1404:  movl_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1413:  movl_i32m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1433:  movl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1451:  movb_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1461:  movb_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1468:  movb_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1475:  movl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1482:  movl_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1489:  movl_rmr0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1497:  movl_i8r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1505:  orl_ir is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1554:  jmp_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1560:  jmp_m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/MacroAssemblerSH4.h:76:  claim_scratch is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/MacroAssemblerSH4.h:81:  release_scratch is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 90 in 7 files


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

[Patrick R. Gansterer]

Can you please set the obsolete flag at the old patch and the patch flag at the new one. This will enable the "Review Patch" Links beside the patch ;-).

I can't say something about the real content, but I saw some possible style improvements:
1) Use ASSERT_NOT_REACHED() instead of ASSERT(0).
   And maybe ASSERT_ARG in the case of "ASSERT((cond == Zero) || (cond == NonZero))"

2) One-line control clauses should not use braces unless comments are included or a single statement spans multiple lines. e.g:
+      } else {
+          m_assembler.xorl_i8r(imm.m_value, srcDest);
+      }

3) You add spaces, where tabs are uses usually:
 	JavaScriptCore/assembler/MacroAssemblerX86.h \
+        JavaScriptCore/assembler/MacroAssemblerSH4.h \
 	JavaScriptCore/assembler/RepatchBuffer.h \

4) Can you please move the header in ExecutableAllocator.h to the top of the file. No reason to add #include in the middle of the code. The code seams linux specific (LINUX_PAGE_SIZE), so let's add " && OS(LINUX)" to "#elif CPU(SH4)"

5) Prefer const to #define. Prefer inline functions to macros.
+#define LINUX_PAGE_SIZE       4096
can be written like
static const unsigend defaultLinuxPageSize = 4096;


IMHO you can create the cacheFlush change as an independet patch. This will make review easier.

[Gabor Loki]

> Created an attachment (id=67675) [details]

Hmmm, you have implemented your own constant/literal pool for SH4. Why? Is the AssemblerBufferWithConstantPool is not sufficent for SH4?

As I see you follow a very similar way to attach the data and pointers into the instruction stream as we have done in AssemblerBufferWithConstantPool. At first sight I did not see any reason why the SH4 should have its own pool implementation. I suggest you try to use the AssemblerBufferWithConstantPool interface for SH4. If you have any problem or question about the interface, you can contact me at IRC.

In addition, I agree with Patrick about the style issue:
- use static const ints, enums and inline functions instead of macros
- fix tabs, unnecessary parenthesis and other trivial style errors which are reported by the check-webkit-style script.

One more question:
What was the problem with the generatePatternCharacterPair function? Why do you have to read a simple character? (I am not familiar with the SH4 architecture)

[thouraya]

Hello,

(In reply to comment #7)
> > Created an attachment (id=67675) [details] [details]
> 
> Hmmm, you have implemented your own constant/literal pool for SH4. Why? Is the AssemblerBufferWithConstantPool is not sufficent for SH4?
> 
> As I see you follow a very similar way to attach the data and pointers into the instruction stream as we have done in AssemblerBufferWithConstantPool. At first sight I did not see any reason why the SH4 should have its own pool implementation. I suggest you try to use the AssemblerBufferWithConstantPool interface for SH4. If you have any problem or question about the interface, you can contact me at IRC.

We implemented our constant pool before you add AssemblerBufferWithConstantPool.

I'll try to use it and let you know.

> 
> In addition, I agree with Patrick about the style issue:
> - use static const ints, enums and inline functions instead of macros
> - fix tabs, unnecessary parenthesis and other trivial style errors which are reported by the check-webkit-style script.
> 
> One more question:
> What was the problem with the generatePatternCharacterPair function? Why do you have to read a simple character? (I am not familiar with the SH4 architecture)

We have to read a simple character because the address should be correctly aligned for a long-word otherwise we'll get a misaligned address.

Regards,
Thouraya.

[thouraya]

(In reply to comment #7)
> > Created an attachment (id=67675) [details] [details]
> 
> Hmmm, you have implemented your own constant/literal pool for SH4. Why? Is the AssemblerBufferWithConstantPool is not sufficent for SH4?
> 
> As I see you follow a very similar way to attach the data and pointers into the instruction stream as we have done in AssemblerBufferWithConstantPool. At first sight I did not see any reason why the SH4 should have its own pool implementation. I suggest you try to use the AssemblerBufferWithConstantPool interface for SH4. If you have any problem or question about the interface, you can contact me at IRC.
> 
> In addition, I agree with Patrick about the style issue:
> - use static const ints, enums and inline functions instead of macros
> - fix tabs, unnecessary parenthesis and other trivial style errors which are reported by the check-webkit-style script.
> 
> One more question:
> What was the problem with the generatePatternCharacterPair function? Why do you have to read a simple character? (I am not familiar with the SH4 architecture)

Hello,

I started to use the AssemblerBufferWithConstantPool :

in arm you are encoding the offset of the constant in the constant pool in the LDR instruction (offset << 1 + 1).

to know if the constant was dumped or no, you have just to see the last bit, if it's 1 the constant is not yet dumped otherwise it's dumped.
 
But in sh4 we are using the mov@PC-relative and the offset can be pair or impair.

that's why we added the offset of the LDR instruction in our structure. 

so, when we dump the constant Pool we patch the LDR instructions.

so to know if the constant is not yet dumped, we need just to look at the offset of the LDR instruction which shoold be 0.



Please rectify if I m wrong.

Ciao,
Thouraya.

[Gabor Loki]

> in arm you are encoding the offset of the constant in the constant pool in the LDR instruction (offset << 1 + 1).
> 
> to know if the constant was dumped or no, you have just to see the last bit, if it's 1 the constant is not yet dumped otherwise it's dumped.

No. The AssemblerBufferWithConstantPool::flushConstantPool function will dump the pool into the instruction stream. In point of fact there is no need any kind of decorator. In case of ARM the last bit decorator is just a legacy helper (from there were no MASM and CP interfaces).

> But in sh4 we are using the mov@PC-relative and the offset can be pair or impair.

It can be done simply with the current AssemblerBufferWithConstantPool. There are two kind of patchConstantPoolLoad functions. One is to store the index of constants/literals, while the real constants are in a vector. The second one is to patch those instructions which hold the indexes with real constants to create the final instructions.

For example:
1) After a putIntWithConstantInt(insn, constant) is called.
 - the constant is saved in a vector
 - a TYPE patchConstantPoolLoad(TYPE load, int value) is called where
  the value is the position of the constant in the pool.
...
2) After the flushConstantPool is called
 - the void patchConstantPoolLoad(void* loadAddr, void* constPoolAddr) is called for each constants/literals in the pool to build the final instruction

You can see there is no pressure to decorate the addresses. If you are brave just store the index with the first patchConstantPoolLoad function and repatch this index with the final instruction. There is only one restriction: the final and the index-holder instructions should have the same length.

(Hmm, we should rename them. It looks like the naming-convention of the patchConstantPoolLoad functions are quite confusing.)

(In reply to comment #10)
> > in arm you are encoding the offset of the constant in the constant pool in the LDR instruction (offset << 1 + 1).
> > 
> > to know if the constant was dumped or no, you have just to see the last bit, if it's 1 the constant is not yet dumped otherwise it's dumped.
> 
> No. The AssemblerBufferWithConstantPool::flushConstantPool function will dump the pool into the instruction stream. In point of fact there is no need any kind of decorator. In case of ARM the last bit decorator is just a legacy helper (from there were no MASM and CP interfaces).
> 
> > But in sh4 we are using the mov@PC-relative and the offset can be pair or impair.
> 
> It can be done simply with the current AssemblerBufferWithConstantPool. There are two kind of patchConstantPoolLoad functions. One is to store the index of constants/literals, while the real constants are in a vector. The second one is to patch those instructions which hold the indexes with real constants to create the final instructions.
> 
> For example:
> 1) After a putIntWithConstantInt(insn, constant) is called.
>  - the constant is saved in a vector
>  - a TYPE patchConstantPoolLoad(TYPE load, int value) is called where
>   the value is the position of the constant in the pool.
> ...
> 2) After the flushConstantPool is called
>  - the void patchConstantPoolLoad(void* loadAddr, void* constPoolAddr) is called for each constants/literals in the pool to build the final instruction
> 
> You can see there is no pressure to decorate the addresses. If you are brave just store the index with the first patchConstantPoolLoad function and repatch this index with the final instruction. There is only one restriction: the final and the index-holder instructions should have the same length.
> 
> (Hmm, we should rename them. It looks like the naming-convention of the patchConstantPoolLoad functions are quite confusing.)


Hello,

My problem comes when I call the function linkJump(JmpSrc from, JmpDst to) to patch the Jump instruction.

the offset for the jump is loaded into a register because we don't know if it can be reached by the jump instruction or not.

if the constant pool  is dumped we have to store the real offset, otherwise we have to  put the correct offset in the vector.

but I don't have a way to know if the constant is dumped or no.
In your case if the offset of the LDR instruction is pair it's dumped otherwise it is not yet dumped.


Regards.

[Gabor Loki]

> if the constant pool  is dumped we have to store the real offset, otherwise we have to  put the correct offset in the vector.
> 
> but I don't have a way to know if the constant is dumped or no.

Well, I have take a short look into the SH4 instruction set.
So, you use mov.l @(disp,PC), Rn to load an address to a register, don't you?
Why don't you patch the instruction in case of not dumped offset (or a dumped one)?

For example:
You can use mov.w @(disp,PC), Rn instead of mov.l until the CP is dumped. Using mov.w is safe, the load16 should not be used to load a pointer to jump on. During the dump you can still patch the instruction to mov.l.

Just check your code! The void movw_mr(int offset, RegisterID base, RegisterID dst) currently is an unused function. So, there is a bit to flip. :)

[thouraya]

Created attachment 69170 [details]
SH4 JIT for YARR 2

Hello,

Attached a new patch including the basic functionality of the SH4 JIT for the YARR module and using the webkit's constant pool.

Using the check-webkit-style script, I have got some errors like "incorrectly named. Don't use underscores in your identifier names" but the same functions exists in X86, should I rename them ?

Regards,
Thouraya.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
aming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1191:  mov_imm8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1198:  movl_rr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1206:  movw_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1214:  movw_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1222:  movw_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1236:  movw_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1243:  movl_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1258:  movl_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1267:  movl_i32m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1287:  movl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1305:  movb_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1315:  movb_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1322:  movb_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1329:  movl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1336:  movl_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1343:  movl_rmr0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1351:  movl_i8r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1359:  orl_ir is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1419:  jmp_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1426:  jmp_m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 86 in 8 files


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

[Gabor Loki]

(In reply to comment #13)
> Created an attachment (id=69170) [details]

It looks like you forgot to set the 'patch' flag on your attachment. You can do it at the 'Details' page.

> Attached a new patch including the basic functionality of the SH4 JIT for the YARR module and using the webkit's constant pool.

Well, the constant pool should be target independent. I do not like the ifdefs at there, but I see why you need that workaround. I will extend the constant pool interface with the required changes. (I have created bug 46796 to follow up that changes.)

[thouraya]

Created attachment 69349 [details]
JIT for YARR 3

Hello,

Attached a new patch after using the patch you provided to extend the constant pool interface.

I'm waiting for you feedback to add the remaining part of JIT.

Regards.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
aming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1197:  mov_imm8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1204:  movl_rr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1212:  movw_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1220:  movw_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1228:  movw_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1242:  movw_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1249:  movl_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1264:  movl_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1273:  movl_i32m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1293:  movl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1311:  movb_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1321:  movb_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1328:  movb_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1335:  movl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1342:  movl_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1349:  movl_rmr0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1357:  movl_i8r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1365:  orl_ir is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1425:  jmp_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/SH4Assembler.h:1432:  jmp_m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 86 in 7 files


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

[Patrick R. Gansterer]

Comment on attachment 69349 [details]
JIT for YARR 3

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

> JavaScriptCore/ChangeLog:10
> +        Add assembler instructions for SH4 platform
> +
> +        * GNUmakefile.am:
> +        * assembler/MacroAssembler.h:
> +        * jit/ExecutableAllocator.h:
> +        (JSC::ExecutableAllocator::cacheFlush):

You do not mention the implementation of chacheFlush for SH4 in the ChangeLog. Maybe you can create a own patch only for the changes in ExecutableAllocator.h.

> JavaScriptCore/jit/ExecutableAllocator.h:310
> +static const unsigned defaultLinuxPageSize = 4096;

IMHO this should be intendet 4 spaces, because it's C code.

[thouraya]

Created attachment 70749 [details]
SH4 JIT 

Hi,

You can find attached all the JIT support for SH4.

I'm waiting for your feedbacks.

Regards,
Thouraya.

[thouraya]

Created attachment 70750 [details]
JIT ENABLE for SH4

Hi,

A new patch enabling JIT for SH4.

Regards,
Thouraya.

[Gabor Loki]

> JavaScriptCore/GNUmakefile.am:544
> +if CPU_SH4
> +javascriptcore_sources += \
> +    JavaScriptCore/jit/JITArithmetic_sh4.cpp
> +else
> +javascriptcore_sources += \
> +    JavaScriptCore/jit/JITArithmetic.cpp
> +endif

Is it really necessary? And what is about JSValue32_64?

> JavaScriptCore/assembler/MacroAssemblerSH4.h:5
> + * Copyright (C) 2009-2010 STMicroelectronics. All rights reserved.
> + * Written by Thouraya Andolsi ([e-mail]).
> + *
> + * Copyright (C) 2008 Apple Inc. All rights reserved.

It is possible that you had SH4 JIT since 2009, but the public version is just released in this year.
The "Written by" line should be another Copyright line instead (for example: Copyright (C) 2010 Thouraya Andolsi <[e-mail]>), and the Copyright lines should be in ascending time order.

> JavaScriptCore/assembler/SH4Assembler.cpp:22
> +void* SH4Assembler::executableCopy(ExecutablePool* allocator)
> +{
> +    void* copy = m_buffer.executableCopy(allocator);
> +
> +    ASSERT(copy);
> +    return copy;
> +}

If this executableCopy just calls its buffer's executableCopy, you should move this function as an inline function into the header.

> JavaScriptCore/assembler/SH4Assembler.h:38
> +#define GETOPCODE1(a, b, c)  ((a) | (((b) & 0xf) << 8) |(((c) & 0xf) << 4))
> +#define GETOPCODE2(a, b)  ((a) | (((b) & 0xf) << 8))

Please, use inline functions and better names (see the formatter of Thumb-2 JIT).

> JavaScriptCore/assembler/SH4Assembler.h:54
> +#define INVALID_OPCODE   0xffff
> +#define ADD_OPCODE       0x300C
> +#define ADDIMM_OPCODE    0x7000
> +#define ADDC_OPCODE      0x300E
> +#define ADDV_OPCODE      0x300F

It would be better using enumerations instead of defines.

> JavaScriptCore/assembler/SH4Assembler.h:270
> +        R14 = 14,
> +        R15 = 15,
> +        SP  = R15,
> +        FP  = R14,

There is no need for all initializers. See:
R0
...
R13,
R14, FP = R14,
R15, SP = R15,
PC,
PR

> JavaScriptCore/assembler/SH4Assembler.h:394
> +        if ((constant <=127) && (constant >= -128))

There is a missing space before 127 constant. The inner parentheses are not needed.

> JavaScriptCore/assembler/SH4Assembler.h:521
> +        ASSERT((imm8 <=127) && (imm8 >= -128));

Again, missing space before 127 and parentheses.

> JavaScriptCore/assembler/SH4Assembler.h:708
> +        oneShortOp(opc);
> +
> +    }

There is unnecessary empty line at the end of a function.

> JavaScriptCore/assembler/SH4Assembler.h:751
> +        case HS: opc = GETOPCODE1(CMPHS_OPCODE, right, left); // HS

I guess this comment is not necessary. ;)

> JavaScriptCore/assembler/SH4Assembler.h:1527
> +            instruction = (0x0023 | (*instructionPtr++ & 0xF00));

We prefer enumerations and static integers instead of magic numbers. Well, it is not necessary for every cases, but it can help to understand the code (especially when you are altering an instruction).

> JavaScriptCore/assembler/SH4Assembler.h:1540
> +            uint16_t* address  = (uint16_t*)((offsetToPC << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));

Use C++ like type conversion (static_cast<>, reinterpret_cast<>, etc.) instead of this C cast!

> JavaScriptCore/assembler/SH4Assembler.h:1763
> +         } else  
> +            ASSERT_NOT_REACHED();

If it is true, you should remove the last 'if' statement and use an ASSERT inside the last (else) block instead of this solution.
For example:
if () {
} else if () {
} else {
  ASSERT(cond);
}

> JavaScriptCore/assembler/SH4Assembler.h:1838
> +        /*
> +        code@  mov.l @(offset , PC) , reg
> +    */  

There is a white-space problem in this comment.

> JavaScriptCore/jit/JIT.cpp:121
>  #else
> +#if CPU(SH4)
> +void JIT::emitTimeoutCheck()
> +{
> +    m_assembler.dt(timeoutCheckRegister);
> +
> +    Jump skipTimeout = Jump(m_assembler.jne());
> +    m_assembler.nop();
> +    m_assembler.nop();
> +    JITStubCall(this, cti_timeout_check).call(timeoutCheckRegister);
> +    skipTimeout.link(this);
> +
> +    killLastResultRegister();
> +}
> +#else
>  void JIT::emitTimeoutCheck()

Why is it necessary? What is SH4's problem about the common solution?

> JavaScriptCore/jit/JIT.h:475
>          void emitJumpSlowCaseIfJSCell(RegisterID);
> +#if CPU(SH4)
> +        Jump emitJumpIfNotJSCell(RegisterID, bool needConstant = true);
> +#else
>          Jump emitJumpIfNotJSCell(RegisterID);
> +#endif
>          void emitJumpSlowCaseIfNotJSCell(RegisterID);

Could you tell us more why these kind of changes are necessary in the common interface? So many changes in the common code!

> JavaScriptCore/jit/JITArithmetic_sh4.cpp:32
> +extern unsigned int __fpscr_values[2];

Hmm, where is the initializer of this? Is it some kind of static array? And what will you do with it if there is multiple JSC thread?


Well, the assembler part of this patch is fine (with some cosmetic changes), but I do not support those *big* changes in the common interface of JIT. I do not think that it is impossible to handle those problems with jumps and literals in the current macroassembler interface. I know the macroassembler is a little bit x86 specific, but ARM (with ARM and Thumb-2 instruction set) and MIPS can also fit this infrastructure well. So, I encourage you to rewrite the MacroAssemblerSH4 interface to reduce the current changes in the common interface. However, Thouraya, you did a great work to implement SH4 JIT, but I think your patch is not acceptable in the current form.

[thouraya]

Hello,

(In reply to comment #21)
> > JavaScriptCore/GNUmakefile.am:544
> > +if CPU_SH4
> > +javascriptcore_sources += \
> > +    JavaScriptCore/jit/JITArithmetic_sh4.cpp
> > +else
> > +javascriptcore_sources += \
> > +    JavaScriptCore/jit/JITArithmetic.cpp
> > +endif
> 
> Is it really necessary? And what is about JSValue32_64?

We started working on webkit in 2009 and we are updating it from the trunk each time.

At that time there was some calls directly to X86 registers, so I prefered to create a customized file for SH4 in order to optimize a bit.

The port is for 32-bit.
We can work on 64-bit , after the 32-bit port is accepted.


> 
> > JavaScriptCore/assembler/MacroAssemblerSH4.h:5
> > + * Copyright (C) 2009-2010 STMicroelectronics. All rights reserved.
> > + * Written by Thouraya Andolsi ([e-mail]).
> > + *
> > + * Copyright (C) 2008 Apple Inc. All rights reserved.
> 
> It is possible that you had SH4 JIT since 2009, but the public version is just released in this year.

We started working on Webkit JIT in 2009, but it was available in 2010.

> The "Written by" line should be another Copyright line instead (for example: Copyright (C) 2010 Thouraya Andolsi <[e-mail]>), and the Copyright lines should be in ascending time order.
> 
> > JavaScriptCore/assembler/SH4Assembler.cpp:22
> > +void* SH4Assembler::executableCopy(ExecutablePool* allocator)
> > +{
> > +    void* copy = m_buffer.executableCopy(allocator);
> > +
> > +    ASSERT(copy);
> > +    return copy;
> > +}
> 
> If this executableCopy just calls its buffer's executableCopy, you should move this function as an inline function into the header.
> 
> > JavaScriptCore/assembler/SH4Assembler.h:38
> > +#define GETOPCODE1(a, b, c)  ((a) | (((b) & 0xf) << 8) |(((c) & 0xf) << 4))
> > +#define GETOPCODE2(a, b)  ((a) | (((b) & 0xf) << 8))
> 
> Please, use inline functions and better names (see the formatter of Thumb-2 JIT).
> 
> > JavaScriptCore/assembler/SH4Assembler.h:54
> > +#define INVALID_OPCODE   0xffff
> > +#define ADD_OPCODE       0x300C
> > +#define ADDIMM_OPCODE    0x7000
> > +#define ADDC_OPCODE      0x300E
> > +#define ADDV_OPCODE      0x300F
> 
> It would be better using enumerations instead of defines.
> 
> > JavaScriptCore/assembler/SH4Assembler.h:270
> > +        R14 = 14,
> > +        R15 = 15,
> > +        SP  = R15,
> > +        FP  = R14,
> 
> There is no need for all initializers. See:
> R0
> ...
> R13,
> R14, FP = R14,
> R15, SP = R15,
> PC,
> PR
> 
> > JavaScriptCore/assembler/SH4Assembler.h:394
> > +        if ((constant <=127) && (constant >= -128))
> 
> There is a missing space before 127 constant. The inner parentheses are not needed.
> 
> > JavaScriptCore/assembler/SH4Assembler.h:521
> > +        ASSERT((imm8 <=127) && (imm8 >= -128));
> 
> Again, missing space before 127 and parentheses.
> 
> > JavaScriptCore/assembler/SH4Assembler.h:708
> > +        oneShortOp(opc);
> > +
> > +    }
> 
> There is unnecessary empty line at the end of a function.
> 
> > JavaScriptCore/assembler/SH4Assembler.h:751
> > +        case HS: opc = GETOPCODE1(CMPHS_OPCODE, right, left); // HS
> 
> I guess this comment is not necessary. ;)

yes of course :)
> 
> > JavaScriptCore/assembler/SH4Assembler.h:1527
> > +            instruction = (0x0023 | (*instructionPtr++ & 0xF00));
> 
> We prefer enumerations and static integers instead of magic numbers. Well, it is not necessary for every cases, but it can help to understand the code (especially when you are altering an instruction).
> 
> > JavaScriptCore/assembler/SH4Assembler.h:1540
> > +            uint16_t* address  = (uint16_t*)((offsetToPC << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));
> 
> Use C++ like type conversion (static_cast<>, reinterpret_cast<>, etc.) instead of this C cast!
> 
> > JavaScriptCore/assembler/SH4Assembler.h:1763
> > +         } else  
> > +            ASSERT_NOT_REACHED();
> 
> If it is true, you should remove the last 'if' statement and use an ASSERT inside the last (else) block instead of this solution.
> For example:
> if () {
> } else if () {
> } else {
>   ASSERT(cond);
> }
> 
> > JavaScriptCore/assembler/SH4Assembler.h:1838
> > +        /*
> > +        code@  mov.l @(offset , PC) , reg
> > +    */  
> 
> There is a white-space problem in this comment.
> 
> > JavaScriptCore/jit/JIT.cpp:121
> >  #else
> > +#if CPU(SH4)
> > +void JIT::emitTimeoutCheck()
> > +{
> > +    m_assembler.dt(timeoutCheckRegister);
> > +
> > +    Jump skipTimeout = Jump(m_assembler.jne());
> > +    m_assembler.nop();
> > +    m_assembler.nop();
> > +    JITStubCall(this, cti_timeout_check).call(timeoutCheckRegister);
> > +    skipTimeout.link(this);
> > +
> > +    killLastResultRegister();
> > +}
> > +#else
> >  void JIT::emitTimeoutCheck()
> 
> Why is it necessary? What is SH4's problem about the common solution?

The common solution should work fine, but with our solution we reduce the number of instructions.

DT is equivalent to subtract one and compare.


> 
> > JavaScriptCore/jit/JIT.h:475
> >          void emitJumpSlowCaseIfJSCell(RegisterID);
> > +#if CPU(SH4)
> > +        Jump emitJumpIfNotJSCell(RegisterID, bool needConstant = true);
> > +#else
> >          Jump emitJumpIfNotJSCell(RegisterID);
> > +#endif
> >          void emitJumpSlowCaseIfNotJSCell(RegisterID);
> 
> Could you tell us more why these kind of changes are necessary in the common interface? So many changes in the common code!

The offset of branch instruction may be huge, so we need to load it in the constantPool especially when jumping to slowcases.

In other cases the offset is reachable and we dont need to load it, that's why I added this parameter.


> 
> > JavaScriptCore/jit/JITArithmetic_sh4.cpp:32
> > +extern unsigned int __fpscr_values[2];
> 
> Hmm, where is the initializer of this? Is it some kind of static array? And what will you do with it if there is multiple JSC thread?
> 
> 
> Well, the assembler part of this patch is fine (with some cosmetic changes), but I do not support those *big* changes in the common interface of JIT. I do not think that it is impossible to handle those problems with jumps and literals in the current macroassembler interface. I know the macroassembler is a little bit x86 specific, but ARM (with ARM and Thumb-2 instruction set) and MIPS can also fit this infrastructure well. So, I encourage you to rewrite the MacroAssemblerSH4 interface to reduce the current changes in the common interface. However, Thouraya, you did a great work to implement SH4 JIT, but I think your patch is not acceptable in the current form.

In the common code, there are a lot of branches and the offsets are known when linking.
when branching to solwcase, the offset is not reachable, so we need to load it in the constantPool, otherwise there is no need. that's why I added in branch functions a parameter to check  if we need to load a constant or not.


Removing the changes done in the common code, webkit should work fine in SH4,
but there are a lot of constants loaded in the constantpool in banch instructions but offsets are reached.

Regards,
Thouraya.

[Csaba Osztrogonác]

Comment on attachment 70749 [details]
SH4 JIT 

r-, because it isn't ready for landing yet (based on https://bugs.webkit.org/show_bug.cgi?id=44329#c21 )

[Csaba Osztrogonác]

Comment on attachment 70750 [details]
JIT ENABLE for SH4

First the SH4 JIT patch should be landed. I removed the r? 
flag, please set it again when the base patch landed.

[thouraya]

(In reply to comment #24)
> (From update of attachment 70750 [details])
> First the SH4 JIT patch should be landed. I removed the r? 
> flag, please set it again when the base patch landed.

Hi,

Should I resubmit an updated version? or reset the review flag to ?

Regards,
Thouraya.

[Csaba Osztrogonác]

(In reply to comment #25)
> Should I resubmit an updated version? or reset the review flag to ?

I just realized your patches should be updated to ToT too to apply.
So after the base patch landed, please update the enabling patch to ToT.

[Gabor Loki]

> We can work on 64-bit , after the 32-bit port is accepted.

There are not so much changes to get JSValue32_64 working (if you have a JSValue32 version).

> The common solution should work fine, but with our solution we reduce the number of instructions.

Well, we have to sacrifice some performance to have a common masm and jit. Unfortunately the same thing is true for each architecture.

> In the common code, there are a lot of branches and the offsets are known when linking.
> when branching to solwcase, the offset is not reachable, so we need to load it in the constantPool, otherwise there is no need. that's why I added in branch functions a parameter to check  if we need to load a constant or not.

On ARMv5 and below the situation is the same, there are lots of pc relative loads which read a pointer or a big number. There are only very few cases when we are able to encode a constant into an instruction (most of those constants appear in the arithmetic part of jit).

[Gavin Barraclough]

I'm afraid I'm a little overloaded, and don't have time to look at this in detail right now, but three key issues stand out:

(1) Unfortunately we've just deprecated JSVALUE32 support, so you will need to update to support JSVALUE32_64.

(2) MacroAssembler is intended to provided a common code generation abstraction across all architectures, providing a single universal interface – we need this interface to line up on all platforms.  The branch* methods need to adhere to the current interface.

(3) In order to be able to move the JIT forwards on all platforms we are keen to see the bulk of the JIT remain architecture-neutral, and common in implementation across all platforms.  We're not looking to include a duplicate set of implementations for all arithmetic opcode specific to any platform.

[Eduardo Felipe Castegnaro]

Sorry to jump in the discussion, but I can't seem to find an SVN revision against which this patch applies cleanly.

I've contacted Thouraya (author of the patch), and he pointed out revision 1.3.3 of WebKitGTK+, but his latest patch has rejects even against that version.

I would like to test it, but after two days trying to apply it and fix the rejects, I'm almost giving up. Could anyone help?

Thanks in advance,
Eduardo.

[STEPHAN Gael]

As indicated in the very beginning of the bug:


"Here attached a first patch generated for the revision 65071 containing..."

[Eduardo Felipe Castegnaro]

(In reply to comment #30)
> As indicated in the very beginning of the bug:
> 
> 
> "Here attached a first patch generated for the revision 65071 containing..."\

I guess I needed someone to point out the obvious for me. As he updated the patches I also imagined he moved the target without trying the first on. Silly of me. Thanks a lot!

[Eduardo Felipe Castegnaro]

After applying SH4 JIT and JIT ENABLE for SH4 patches cleanly on 65071, and Constant Pool patch with a single change, I'm still having errors compiling:

JavaScriptCore/jit/JITArithmetic.cpp: In member function ‘void JSC::JIT::emit_op_rshift(JSC::Instruction*)’:
JavaScriptCore/jit/JITArithmetic.cpp:123: error: no matching function for call to ‘JSC::JIT::loadDouble(JSC::AbstractMacroAssembler<JSC::SH4Assembler>::Address, const JSC::SH4::FPRegisterID&)’
JavaScriptCore/assembler/MacroAssemblerSH4.h:517: note: candidates are: void JSC::MacroAssemblerSH4::loadDouble(JSC::AbstractMacroAssembler<JSC::SH4Assembler>::ImplicitAddress, JSC::SH4::FPRegisterID, JSC::SH4::RegisterID)
JavaScriptCore/assembler/MacroAssemblerSH4.h:522: note:                 void JSC::MacroAssemblerSH4::loadDouble(JSC::SH4::RegisterID, JSC::SH4::FPRegisterID)
JavaScriptCore/jit/JITArithmetic.cpp: In member function ‘void JSC::JIT::emit_op_urshift(JSC::Instruction*)’:
JavaScriptCore/jit/JITArithmetic.cpp:203: error: ‘urshift32’ was not declared in this scope
JavaScriptCore/jit/JITArithmetic.cpp:223: error: ‘urshift32’ was not declared in this scope


urshift32 is defined for ARM, MIPS and x86, but not for SH4. And the fist no matching call I have no idea how to adapt. Any thoughts? Is 65071 the right version for this patch?

Hello,

Applying those patches, you should have a file called JITArithmetic_sh4.cpp
You should use it instead of JITArithmetic.cpp.

It should be included in JavaScriptCore/GNUmakefile.am

Regards,
Thouraya.

(In reply to comment #32)
> After applying SH4 JIT and JIT ENABLE for SH4 patches cleanly on 65071, and Constant Pool patch with a single change, I'm still having errors compiling:
> 
> JavaScriptCore/jit/JITArithmetic.cpp: In member function ‘void JSC::JIT::emit_op_rshift(JSC::Instruction*)’:
> JavaScriptCore/jit/JITArithmetic.cpp:123: error: no matching function for call to ‘JSC::JIT::loadDouble(JSC::AbstractMacroAssembler<JSC::SH4Assembler>::Address, const JSC::SH4::FPRegisterID&)’
> JavaScriptCore/assembler/MacroAssemblerSH4.h:517: note: candidates are: void JSC::MacroAssemblerSH4::loadDouble(JSC::AbstractMacroAssembler<JSC::SH4Assembler>::ImplicitAddress, JSC::SH4::FPRegisterID, JSC::SH4::RegisterID)
> JavaScriptCore/assembler/MacroAssemblerSH4.h:522: note:                 void JSC::MacroAssemblerSH4::loadDouble(JSC::SH4::RegisterID, JSC::SH4::FPRegisterID)
> JavaScriptCore/jit/JITArithmetic.cpp: In member function ‘void JSC::JIT::emit_op_urshift(JSC::Instruction*)’:
> JavaScriptCore/jit/JITArithmetic.cpp:203: error: ‘urshift32’ was not declared in this scope
> JavaScriptCore/jit/JITArithmetic.cpp:223: error: ‘urshift32’ was not declared in this scope
> 
> 
> urshift32 is defined for ARM, MIPS and x86, but not for SH4. And the fist no matching call I have no idea how to adapt. Any thoughts? Is 65071 the right version for this patch?

[Oliver Hunt]

(In reply to comment #33)
> Hello,
> 
> Applying those patches, you should have a file called JITArithmetic_sh4.cpp
> You should use it instead of JITArithmetic.cpp.
> 
> It should be included in JavaScriptCore/GNUmakefile.am
> 
> Regards,
> Thouraya.
> 

There should not be a new jitarithmetic file.  The only arch dependent files are the assemblers.

I will r- any patch that attempts to add arch dependent logic to the jit (beyond calling convention stuff obviously).

[Oliver Hunt]

Looking at this code it is clear that there is just _way_ to much new conditional code being added to cross platform files.  It's just not acceptable.   If your assembler implementations are set up correctly there should be no need for crossplatform code to be modified.

[thouraya]

(In reply to comment #35)
> Looking at this code it is clear that there is just _way_ to much new conditional code being added to cross platform files.  It's just not acceptable.   If your assembler implementations are set up correctly there should be no need for crossplatform code to be modified.

Hello,

I added the missed functions in macroassembler, removed the code that I added in the common part and added the support for JSVALUE32_64 since  JSVALUE32  is deprecated.

I'm doing some tests before adding the new patch.

Regards,
Thouraya.

[Eduardo Felipe Castegnaro]

Any news on this?

When I finally managed to apply this patch to the correct version of WebKit and modify my build system (which is CMake), compile and run it, I got a lot of unaligned accesses and SunSpider performance, when compared to the version present on http://www.stlinux.com, was subpar (about a third).

Best regards,
Eduardo.

(In reply to comment #36)
> (In reply to comment #35)
> > Looking at this code it is clear that there is just _way_ to much new conditional code being added to cross platform files.  It's just not acceptable.   If your assembler implementations are set up correctly there should be no need for crossplatform code to be modified.
> 
> Hello,
> 
> I added the missed functions in macroassembler, removed the code that I added in the common part and added the support for JSVALUE32_64 since  JSVALUE32  is deprecated.
> 
> I'm doing some tests before adding the new patch.
> 
> Regards,
> Thouraya.

Hello,

I fixed those problems.
I'm preparing a patch to support JSValue32-64, and fix will be there.

Ciao,


(In reply to comment #37)
> Any news on this?
> 
> When I finally managed to apply this patch to the correct version of WebKit and modify my build system (which is CMake), compile and run it, I got a lot of unaligned accesses and SunSpider performance, when compared to the version present on http://www.stlinux.com, was subpar (about a third).
> 
> Best regards,
> Eduardo.
> 
> (In reply to comment #36)
> > (In reply to comment #35)
> > > Looking at this code it is clear that there is just _way_ to much new conditional code being added to cross platform files.  It's just not acceptable.   If your assembler implementations are set up correctly there should be no need for crossplatform code to be modified.
> > 
> > Hello,
> > 
> > I added the missed functions in macroassembler, removed the code that I added in the common part and added the support for JSVALUE32_64 since  JSVALUE32  is deprecated.
> > 
> > I'm doing some tests before adding the new patch.
> > 
> > Regards,
> > Thouraya.

Created attachment 76199 [details]
JIT support JSValue32-64

Hi,

Attached new patche to add JIT support for revision 71224 for webkit version 1.3.5 supporting JSVALUE32_64.

Regards,
Thouraya.

[thouraya]

Created attachment 76200 [details]
Enable JIT

Enable JIT build.

[Oliver Hunt]

(In reply to comment #39)
> Created an attachment (id=76199) [details]
> JIT support JSValue32-64
> 
> Hi,
> 
> Attached new patche to add JIT support for revision 71224 for webkit version 1.3.5 supporting JSVALUE32_64.
> 
> Regards,
> Thouraya.

Errr, why are you targeting such an old revision -- we aren't going to land a patch that is esssentially very old.  The patch needs to be against ToT.

[Eduardo Felipe Castegnaro]

(In reply to comment #39)
> Created an attachment (id=76199) [details]
> JIT support JSValue32-64
> 
> Hi,
> 
> Attached new patche to add JIT support for revision 71224 for webkit version 1.3.5 supporting JSVALUE32_64.

Were you able to successfully run this patch by just downloading the SVN revision 71224 and applying the constant pool patch?

Your patch applies cleanly, but builds with two warnings (signed-unsigned comparison and no return in function returning value) and I get segmentation fault on the JavaScript shell application (jsc) when it finds something that can be JITed (like loops with mathematical expressions inside them).

Debugging let me to believe that this is happening due to JIT code putting code not at 32 bit boundaries.

Here is what I'm getting:

bash-3.00# jsc
> a = 5;
5
> a
5
> for (count=1; count<=100; count=count+1) { a = a + count; }
Unaligned userspace access in "jsc" pid=851 pc=0x2b48ae24 ins=0x6332
Sending SIGBUS to "jsc" due to unaligned access (PC 2b48ae24 PR 2968f85a)

The kernel on STLinux has this comment on the function that handles unaligned accesses:

fixup:
        /* unaligned PC is not something we can fix */
        if (regs->pc & 1) {
            si_code = BUS_ADRALN;
            goto uspace_segv;
        }

[...]

uspace_segv:
        printk(KERN_NOTICE "Sending SIGBUS to \"%s\" due to unaligned "
               "access (PC %lx PR %lx)\n", current->comm, regs->pc,
               regs->pr);


Has anyone seen a different behavior with this patch?

Best regards,

Eduardo.
 
> Regards,
> Thouraya.

[thouraya]

Hello,

I have another patch for flushConstantPool function.

In SH4, the size of the instruction is 16 bits.
So, when we need to jump around the constantpool we shoold emit 2 instructions BRA and NOP (otherwise we will get a misaligned access address)


        if (useBarrier) {
            putIntegral(AssemblerType::placeConstantPoolBarrier(m_numConsts * sizeof(uint32_t) + alignPool));
#if CPU(SH4)
            // otherwise we will get a misaligned address
            AssemblerBuffer::putShort(AssemblerType::padForAlign16);
#endif
        }

Regards.

(In reply to comment #42)
> (In reply to comment #39)
> > Created an attachment (id=76199) [details] [details]
> > JIT support JSValue32-64
> > 
> > Hi,
> > 
> > Attached new patche to add JIT support for revision 71224 for webkit version 1.3.5 supporting JSVALUE32_64.
> 
> Were you able to successfully run this patch by just downloading the SVN revision 71224 and applying the constant pool patch?
> 
> Your patch applies cleanly, but builds with two warnings (signed-unsigned comparison and no return in function returning value) and I get segmentation fault on the JavaScript shell application (jsc) when it finds something that can be JITed (like loops with mathematical expressions inside them).
> 
> Debugging let me to believe that this is happening due to JIT code putting code not at 32 bit boundaries.
> 
> Here is what I'm getting:
> 
> bash-3.00# jsc
> > a = 5;
> 5
> > a
> 5
> > for (count=1; count<=100; count=count+1) { a = a + count; }
> Unaligned userspace access in "jsc" pid=851 pc=0x2b48ae24 ins=0x6332
> Sending SIGBUS to "jsc" due to unaligned access (PC 2b48ae24 PR 2968f85a)
> 
> The kernel on STLinux has this comment on the function that handles unaligned accesses:
> 
> fixup:
>         /* unaligned PC is not something we can fix */
>         if (regs->pc & 1) {
>             si_code = BUS_ADRALN;
>             goto uspace_segv;
>         }
> 
> [...]
> 
> uspace_segv:
>         printk(KERN_NOTICE "Sending SIGBUS to \"%s\" due to unaligned "
>                "access (PC %lx PR %lx)\n", current->comm, regs->pc,
>                regs->pr);
> 
> 
> Has anyone seen a different behavior with this patch?
> 
> Best regards,
> 
> Eduardo.
> 
> > Regards,
> > Thouraya.

[Eduardo Felipe Castegnaro]

(In reply to comment #43)
> Hello,
> 
> I have another patch for flushConstantPool function.
> 
> In SH4, the size of the instruction is 16 bits.
> So, when we need to jump around the constantpool we shoold emit 2 instructions BRA and NOP (otherwise we will get a misaligned access address)
> 
> 
>         if (useBarrier) {
>             putIntegral(AssemblerType::placeConstantPoolBarrier(m_numConsts * sizeof(uint32_t) + alignPool));
> #if CPU(SH4)
>             // otherwise we will get a misaligned address
>             AssemblerBuffer::putShort(AssemblerType::padForAlign16);
> #endif
>         }
> 
> Regards.

With this fix I've managed to run SunSpider and get a great performance! Better than the version available on STLinux. Thanks so much for your help in getting this running.

Best regards,

Eduardo.

> (In reply to comment #42)
> > (In reply to comment #39)
> > > Created an attachment (id=76199) [details] [details] [details]
> > > JIT support JSValue32-64
> > > 
> > > Hi,
> > > 
> > > Attached new patche to add JIT support for revision 71224 for webkit version 1.3.5 supporting JSVALUE32_64.
> > 
> > Were you able to successfully run this patch by just downloading the SVN revision 71224 and applying the constant pool patch?
> > 
> > Your patch applies cleanly, but builds with two warnings (signed-unsigned comparison and no return in function returning value) and I get segmentation fault on the JavaScript shell application (jsc) when it finds something that can be JITed (like loops with mathematical expressions inside them).
> > 
> > Debugging let me to believe that this is happening due to JIT code putting code not at 32 bit boundaries.
> > 
> > Here is what I'm getting:
> > 
> > bash-3.00# jsc
> > > a = 5;
> > 5
> > > a
> > 5
> > > for (count=1; count<=100; count=count+1) { a = a + count; }
> > Unaligned userspace access in "jsc" pid=851 pc=0x2b48ae24 ins=0x6332
> > Sending SIGBUS to "jsc" due to unaligned access (PC 2b48ae24 PR 2968f85a)
> > 
> > The kernel on STLinux has this comment on the function that handles unaligned accesses:
> > 
> > fixup:
> >         /* unaligned PC is not something we can fix */
> >         if (regs->pc & 1) {
> >             si_code = BUS_ADRALN;
> >             goto uspace_segv;
> >         }
> > 
> > [...]
> > 
> > uspace_segv:
> >         printk(KERN_NOTICE "Sending SIGBUS to \"%s\" due to unaligned "
> >                "access (PC %lx PR %lx)\n", current->comm, regs->pc,
> >                regs->pr);
> > 
> > 
> > Has anyone seen a different behavior with this patch?
> > 
> > Best regards,
> > 
> > Eduardo.
> > 
> > > Regards,
> > > Thouraya.

[Gavin Barraclough]

Hi thouraya,

Looks like this patch is making really great progress.  I'm afraid we're all up against some deadlines here, so I don't think I'll be able to give this patch this patch a proper review immediately, I'll get back to you on this as soon as I can.

G.

[thouraya]

Hi,

it's not a patch,
it's a work-around,

I submitted a patch here https://bugs.webkit.org/attachment.cgi

Regards,

(In reply to comment #45)
> Hi thouraya,
> 
> Looks like this patch is making really great progress.  I'm afraid we're all up against some deadlines here, so I don't think I'll be able to give this patch this patch a proper review immediately, I'll get back to you on this as soon as I can.
> 
> G.

[thouraya]

here https://bugs.webkit.org/show_bug.cgi?id=44329

(In reply to comment #46)
> Hi,
> 
> it's not a patch,
> it's a work-around,
> 
> I submitted a patch here https://bugs.webkit.org/attachment.cgi
> 
> Regards,
> 
> (In reply to comment #45)
> > Hi thouraya,
> > 
> > Looks like this patch is making really great progress.  I'm afraid we're all up against some deadlines here, so I don't think I'll be able to give this patch this patch a proper review immediately, I'll get back to you on this as soon as I can.
> > 
> > G.

[thouraya]

I'm sorry,
the patch is here https://bugs.webkit.org/show_bug.cgi?id=46796
(In reply to comment #47)
> here https://bugs.webkit.org/show_bug.cgi?id=44329
> 
> (In reply to comment #46)
> > Hi,
> > 
> > it's not a patch,
> > it's a work-around,
> > 
> > I submitted a patch here https://bugs.webkit.org/attachment.cgi
> > 
> > Regards,
> > 
> > (In reply to comment #45)
> > > Hi thouraya,
> > > 
> > > Looks like this patch is making really great progress.  I'm afraid we're all up against some deadlines here, so I don't think I'll be able to give this patch this patch a proper review immediately, I'll get back to you on this as soon as I can.
> > > 
> > > G.

[thouraya]

Created attachment 76513 [details]
ADD JIT for SH4 platform

Hi,

Attached a new patch , revision 73890, webkit version 1.3.7.

Regards,
Thouraya.

[thouraya]

Created attachment 76514 [details]
JIT Enable

Enable JIT.

[thouraya]

Hi,

Could you kindly give me an idea about the date that you can review the patch.

Is there any plan for webkit1.4 ?

Regards,
Thouraya.

[thouraya]

Hi,

Does any one had a look on my patches ?

I'm still waiting for your feed-backs.

Regards.

[Giuseppe Di Giore]

Hello,

anyone looking at this bug ?

It has not been assigned...

How to speed up the review process for this patch ?

I really need to have the SH4/ST40 JT compiler integrated.

Kind regards,
Giuseppe

[Patrick R. Gansterer]

(In reply to comment #53)
> It has not been assigned...
Most of the bugs in WebKit from contributers are unassigned, this does not matter.

> How to speed up the review process for this patch ?
Make smaller patches:
1) The cacheFlush stuff is completely unrelated to the rest of the patch.
2) see comment #3: Split the patch up in the YARR part and the remaining JIT part.

[Oliver Hunt]

(In reply to comment #52)
> Hi,
> 
> Does any one had a look on my patches ?
> 
> I'm still waiting for your feed-backs.
> 
> Regards.

Eek, sorry for the delay Gavin and I have been really busy for the last few weeks so I apologise for not getting around to reviewing.

I'll try to have a look today

[Oliver Hunt]

Comment on attachment 76513 [details]
ADD JIT for SH4 platform

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

An earlier comment requested that you split this up into two parts, the first being to bring up YARR, the second bringing in the rest of the jit.   I'd lean towards doing this by removing the double operation implementations from your current patch (saving them first of course) which would remove the bulk of the unnecessary logic for YARR.  and keeping the JIT specific changes separate.

There are also a fairly substantial number of style errors - multiple spaces after =, the use of C casts is generally not allowed in C++ code in the webkit project (there are places where it's there, but we're slowly killing them all).  Basically the style bot should be green for a patch to be accepted (depending on the exact complaints we may consider it a style bot bug as asm-y code in the jit tends to be quite different from the rest of the project and needs a few quirks in the rules, but they should all be there by now).

Anyway, r- for now.  The biggest issues I can see are the share patch size (it's difficult to effectively review such a giant patch) which will hopefully be improved by splitting out the YARR parts from the main JSC JIT;  The addition of cpu specific ifdefs to the middle of YARR codegen -- that's not cool; and the various bits of duplicate code.

To update your patch you'll need to deal with the mighty source move of pain where JavaScriptCore (the directory) got moved to Source/JavaScriptCore.  You may find it easiest simply to create the patch from your current tree, then do a search/replace for /JavaScriptCore/ and replace it with /Source/JavaScriptCore.  If you're using git (and git can't deal with the move automatically) then you can use git format-patch to create a set of patches (one for each revision), then do a rename on each of them (sed works wonders) and then use git am to apply them to a ToT branch.  Essentially this is a manual rebase.

> JavaScriptCore/assembler/MacroAssemblerSH4.h:588
> +    bool supportsFloatingPoint() const { return false; }
> +    bool supportsFloatingPointTruncate() const { return false; }
> +    bool supportsFloatingPointSqrt() const { return false; }

You return false from these functions, yet have a bunch of code to handle floating point instructions, why?

> JavaScriptCore/assembler/SH4Assembler.h:1422
> +    void LoadConstant(uint32_t constant, RegisterID dst, bool withpatch = false)
> +    {
> +        if (((int)constant <=0x7f) && ((int)constant >= -0x80) && (!withpatch))
> +            mov_imm8(constant, dst);
> +        else {
> +            m_buffer.ensureSpace(maxInstructionSize, sizeof(uint32_t));
> +            LoadConstantUncheckedSize(constant, dst);
> +        }
> +    }
> +    
> +    void LoadConstantUncheckedSize(uint32_t constant, RegisterID dst)
> +    {
> +        uint16_t opc = GETOPCODEGROUPE3(MOVIMM_OPCODE, dst, 0); // will be patched later to mov.l
> +        m_buffer.putShortWithConstantInt(opc, constant);
> +    }

All method names should start lower cased -- so loadConstant, loadConstantUncheckedSize

> JavaScriptCore/assembler/SH4Assembler.h:1555
> +            uint16_t* address  = (uint16_t*)((offset << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));

C cast

> JavaScriptCore/assembler/SH4Assembler.h:1573
> +            uint16_t* address  = (uint16_t*)((offsetToPC << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));

C cast

> JavaScriptCore/assembler/SH4Assembler.h:1586
> +        uint16_t* address  = (uint16_t*)((offset << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));

This address computation logic is repeated quite a bit.  I'd prefer to see a function to compute this rather than repeated copies of the code.

> JavaScriptCore/jit/JITPropertyAccess32_64.cpp:488
> +    if ((dst >= 0) && (dst <= 7))
> +        END_UNINTERRUPTED_SEQUENCE(sequenceGetByIdSlowCase);
> +    else if ((dst > 15) || (dst < -16))
> +        END_UNINTERRUPTED_SEQUENCES(sequenceGetByIdSlowCase, 8, 2);
> +    else
> +        END_UNINTERRUPTED_SEQUENCES(sequenceGetByIdSlowCase, 8, 0);

Arbitrary constants are bad,  I have no idea what these mean they should be given meaningful descriptions.

> JavaScriptCore/yarr/RegexJIT.cpp:1253
> +#if !CPU(SH4)

There shouldn't be any cpu specific code in the middle of YARR codegen

> JavaScriptCore/yarr/RegexJIT.cpp:1818
> +#if !CPU(SH4)
>                          generatePatternCharacterPair(state);
>                          state.nextTerm();
> +#else
> +                        generatePatternCharacterSingle(state);
> +#endif

You shouldn't be doing anything CPU specific in the middle of YARR codegen.

[Oliver Hunt]

Comment on attachment 76514 [details]
JIT Enable

clearing the review flag

[Balazs Kelemen]

> To update your patch you'll need to deal with the mighty source move of pain where JavaScriptCore (the directory) got moved to Source/JavaScriptCore.  You may find it easiest simply to create the patch from your current tree, then do a search/replace for /JavaScriptCore/ and replace it with /Source/JavaScriptCore.  If you're using git (and git can't deal with the move automatically) then you can use git format-patch to create a set of patches (one for each revision), then do a rename on each of them (sed works wonders) and then use git am to apply them to a ToT branch.  Essentially this is a manual rebase.

svn-apply can handle the move so I rather recommend you to use that.

[Giuseppe Di Giore]

Hello,

many thanks to have quickly provided feedback!

We are going to split and review  the patches as suggested.

I hope to have new patches available early next week.

Thanks again.

Cheers,
Giuseppe

[thouraya]

Hello,

(In reply to comment #56)
> (From update of attachment 76513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76513&action=review
> 
> An earlier comment requested that you split this up into two parts, the first being to bring up YARR, the second bringing in the rest of the jit.   I'd lean towards doing this by removing the double operation implementations from your current patch (saving them first of course) which would remove the bulk of the unnecessary logic for YARR.  and keeping the JIT specific changes separate.
> 
> There are also a fairly substantial number of style errors - multiple spaces after =, the use of C casts is generally not allowed in C++ code in the webkit project (there are places where it's there, but we're slowly killing them all).  Basically the style bot should be green for a patch to be accepted (depending on the exact complaints we may consider it a style bot bug as asm-y code in the jit tends to be quite different from the rest of the project and needs a few quirks in the rules, but they should all be there by now).
> 
> Anyway, r- for now.  The biggest issues I can see are the share patch size (it's difficult to effectively review such a giant patch) which will hopefully be improved by splitting out the YARR parts from the main JSC JIT;  The addition of cpu specific ifdefs to the middle of YARR codegen -- that's not cool; and the various bits of duplicate code.
> 
> To update your patch you'll need to deal with the mighty source move of pain where JavaScriptCore (the directory) got moved to Source/JavaScriptCore.  You may find it easiest simply to create the patch from your current tree, then do a search/replace for /JavaScriptCore/ and replace it with /Source/JavaScriptCore.  If you're using git (and git can't deal with the move automatically) then you can use git format-patch to create a set of patches (one for each revision), then do a rename on each of them (sed works wonders) and then use git am to apply them to a ToT branch.  Essentially this is a manual rebase.
> 
> > JavaScriptCore/assembler/MacroAssemblerSH4.h:588
> > +    bool supportsFloatingPoint() const { return false; }
> > +    bool supportsFloatingPointTruncate() const { return false; }
> > +    bool supportsFloatingPointSqrt() const { return false; }
> 
> You return false from these functions, yet have a bunch of code to handle floating point instructions, why?

I set them to false because floating point is not fully tested.
> 
> > JavaScriptCore/assembler/SH4Assembler.h:1422
> > +    void LoadConstant(uint32_t constant, RegisterID dst, bool withpatch = false)
> > +    {
> > +        if (((int)constant <=0x7f) && ((int)constant >= -0x80) && (!withpatch))
> > +            mov_imm8(constant, dst);
> > +        else {
> > +            m_buffer.ensureSpace(maxInstructionSize, sizeof(uint32_t));
> > +            LoadConstantUncheckedSize(constant, dst);
> > +        }
> > +    }
> > +    
> > +    void LoadConstantUncheckedSize(uint32_t constant, RegisterID dst)
> > +    {
> > +        uint16_t opc = GETOPCODEGROUPE3(MOVIMM_OPCODE, dst, 0); // will be patched later to mov.l
> > +        m_buffer.putShortWithConstantInt(opc, constant);
> > +    }
> 
> All method names should start lower cased -- so loadConstant, loadConstantUncheckedSize
> 
> > JavaScriptCore/assembler/SH4Assembler.h:1555
> > +            uint16_t* address  = (uint16_t*)((offset << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));
> 
> C cast
> 
> > JavaScriptCore/assembler/SH4Assembler.h:1573
> > +            uint16_t* address  = (uint16_t*)((offsetToPC << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));
> 
> C cast
> 
> > JavaScriptCore/assembler/SH4Assembler.h:1586
> > +        uint16_t* address  = (uint16_t*)((offset << 2) + (((unsigned int)instructionPtr + 4) &(~0x3)));
> 
> This address computation logic is repeated quite a bit.  I'd prefer to see a function to compute this rather than repeated copies of the code.
> 
> > JavaScriptCore/jit/JITPropertyAccess32_64.cpp:488
> > +    if ((dst >= 0) && (dst <= 7))
> > +        END_UNINTERRUPTED_SEQUENCE(sequenceGetByIdSlowCase);
> > +    else if ((dst > 15) || (dst < -16))
> > +        END_UNINTERRUPTED_SEQUENCES(sequenceGetByIdSlowCase, 8, 2);
> > +    else
> > +        END_UNINTERRUPTED_SEQUENCES(sequenceGetByIdSlowCase, 8, 0);
> 
> Arbitrary constants are bad,  I have no idea what these mean they should be given meaningful descriptions.

Here we have to store payload and tag in the frame.

to store one of them :
if ((dst >= 0) && (dst <= 7)) we can store it using 1 instruction
if ((dst > 15) || (dst < -16)) we have to load offset in the constantpool => we are emitting  +2 instructions (of 16 bits) and +1 constant in the pool.
otherwise  we have to store dst into a register => +2 instructions.

there is a solution which is to load always dst in the constantpool and emit the same instructions.

but it will slow down the performance.


> 
> > JavaScriptCore/yarr/RegexJIT.cpp:1253
> > +#if !CPU(SH4)
> 
> There shouldn't be any cpu specific code in the middle of YARR codegen
> 
> > JavaScriptCore/yarr/RegexJIT.cpp:1818
> > +#if !CPU(SH4)
> >                          generatePatternCharacterPair(state);
> >                          state.nextTerm();
> > +#else
> > +                        generatePatternCharacterSingle(state);
> > +#endif
> 
> You shouldn't be doing anything CPU specific in the middle of YARR codegen.

We have to read a simple character because the address should be correctly aligned for a long-word otherwise we'll get a misaligned address.

Is there a way to not call generatePatternCharacterPair without this if def ?

[thouraya]

Created attachment 82973 [details]
cacheFlush implementation

A patch for cacheFlush implementation.

[Patrick R. Gansterer]

Comment on attachment 82973 [details]
cacheFlush implementation

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

> Source/JavaScriptCore/jit/ExecutableAllocator.h:326
> +#ifndef __NR_cacheflush
> +#define __NR_cacheflush __NR_modify_ldt
> +#endif
> +
> +static const unsigned defaultLinuxPageSize = 4096;

Please move this lines to the other #defines at the top of the file. 
Can you use getpagesize() instead? Or at least add an assertion?

> Source/JavaScriptCore/jit/ExecutableAllocator.h:332
> +        unsigned int a, b;
> +        a = reinterpret_cast<unsigned int>(code) & ~(defaultLinuxPageSize - 1);
> +        b = (reinterpret_cast<unsigned int>(code) + size + (defaultLinuxPageSize - 1)) & ~(defaultLinuxPageSize - 1);

Use better names for your variables: e.g. currentAddress and endAddress. Also do the variable declaration and assignment in one step:
unsigned int currentAddress = reinterpret_cast<unsigned int>(code) & ~(defaultLinuxPageSize - 1);
unsigned int endAddress = (reinterpret_cast<unsigned int>(code) + size + (defaultLinuxPageSize - 1)) & ~(defaultLinuxPageSize - 1);

Is the memory alignment required?

> Source/JavaScriptCore/jit/ExecutableAllocator.h:342
> +        int res = syscall(__NR_cacheflush, a, defaultLinuxPageSize, CACHEFLUSH_D_WB | CACHEFLUSH_I);
> +        ASSERT(!res);
> +
> +        while (b - a > defaultLinuxPageSize) {
> +            a = a + defaultLinuxPageSize;
> +            res = syscall(__NR_cacheflush, a, defaultLinuxPageSize, CACHEFLUSH_D_WB | CACHEFLUSH_I);
> +
> +            ASSERT(!res);
> +        }

You do the syscall twice with the same arguments. Can you rewrite it with a do while instead?


BTW: Did you try if a simple cachflush works on SH4?
The man page seams outdated: http://git.kernel.org/?p=docs/man-pages/man-pages.git;a=blob;f=man2/cacheflush.2;h=426331ba12646838880901afb4f4215762491ea3;hb=c84860e9ca1c89bfc42591d3320e568d4656f1d4#l76

[thouraya]

Created attachment 83336 [details]
CacheFlush

Hi,

CacheFlush modified according to the comment above.

Regards,
Thouraya.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/jit/ExecutableAlloca..." exit_code: 1

Source/JavaScriptCore/jit/ExecutableAllocator.h:324:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 1 files


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

[Patrick R. Gansterer]

Comment on attachment 83336 [details]
CacheFlush

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

ChangeLog is missing.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:321
> +

No need for this empty line.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:325
> +        ASSERT(!res);

It's better to use ASSERT_UNUSED here.

[thouraya]

Created attachment 83614 [details]
CacheFlush

[Nikolas Zimmermann]

Comment on attachment 83614 [details]
CacheFlush

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

Almost there, can you upload another one fixing my few nitpicks, I'll r+/cq+ it.

> Source/JavaScriptCore/ChangeLog:5
> +        cacheFlush for SH4.

Provide an ExecutableAllocater::cacheFlush() implementation for Linux/SH4.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:321
> +

One newline too much.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:327
> +        syscall(__NR_cacheflush, reinterpret_cast<unsigned int>(code), size, CACHEFLUSH_D_WB | CACHEFLUSH_I | CACHEFLUSH_D_L2);
> +#else
> +        syscall(__NR_cacheflush, reinterpret_cast<unsigned int>(code), size, CACHEFLUSH_D_WB | CACHEFLUSH_I);

s/unsigned int/unsigned/

[Nikolas Zimmermann]

Comment on attachment 83614 [details]
CacheFlush

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

> Source/JavaScriptCore/ChangeLog:4
> +

Forgot to say: Bug number is misisng, please use prepare-ChangeLog --bug=<yourbug> and regenerate the ChangeLog, otherwhise we can't land it.

[thouraya]

Created attachment 83626 [details]
CacheFlush implementation

Hi,
using prepare-ChangeLog --bug=<44329> command, I get the following error "Bug 44329 has no bug description. Maybe you set wrong bug ID?"

So, I added bug description manually.

Is it OK?
Regards.

[Nikolas Zimmermann]

Comment on attachment 83626 [details]
CacheFlush implementation

r=me. Note, that "prepare-ChangeLog --bug=44329" works for me, did you writeout the < > ?

[WebKit Commit Bot]

Comment on attachment 83626 [details]
CacheFlush implementation

Clearing flags on attachment: 83626

Committed r79773: <http://trac.webkit.org/changeset/79773>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[STEPHAN Gael]

I may be wrong, but it looks like the only patch to have landed in the commit is the CacheFlush implementation ( http://trac.webkit.org/changeset/79773 ) and not the JIT ?

[Patrick R. Gansterer]

(In reply to comment #73)
> I may be wrong, but it looks like the only patch to have landed in the commit is the CacheFlush implementation ( http://trac.webkit.org/changeset/79773 ) and not the JIT ?
The commit-queue will close the bug, if there are no patches with a r? or r+: "All reviewed patches have been landed.  Closing bug."

[Eric Seidel (no email)]

Generally we try to encourage one-patch-per-bug and use of master bugs.  But you're welcome to do as you wish.  The tools are designed for the one-patch case however. :)

[thouraya]

Created attachment 84214 [details]
YARR JIT

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Last 3072 characters of output:
r names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1295:  movw_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1302:  movl_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1317:  movl_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1326:  movl_i32m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1344:  movl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1361:  movb_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1370:  movb_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1376:  movb_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1382:  movl_mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1388:  movl_r0mr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1394:  movl_rmr0 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1401:  movl_i8r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1409:  orl_ir is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1479:  jmp_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1486:  jmp_m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1496:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1498:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1740:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/SH4Assembler.h:1760:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/MacroAssembler.h:55:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 94 in 8 files


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

[Patrick R. Gansterer]

Comment on attachment 84214 [details]
YARR JIT 

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

I only looked at the style of this patch and added some comments, but in general:
1) Use 4 space indentation everywhere
2) Use early return
3) Remove the unneeded empty lines
4) Please use singe line comments: Two slashes followed by a space. Comment should be a full sentence with a "." at the end.

Running check-webkit-style will show you many of the style errors.

> Source/JavaScriptCore/ChangeLog:8
> +        Add YARR support for SH4 platforms (disabled by default).

Please remove the function names from ChangeLog if you don't add anything.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.cpp:3
> +/*
> + * Copyright (C) 2011 STMicroelectronics. All rights reserved.
> +*/

Please add a full header with LGPL or BDS license.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.cpp:60
> +}
> +} // namespace JSC

Missing empty line

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:330
> +      if ((srcDest != SH4Registers::r0) || (imm.m_value > 255) || (imm.m_value < 0)) {
> +          RegisterID scr = claimScratch();
> +          m_assembler.loadConstant((imm.m_value), scr);
> +          m_assembler.xorl_rr(scr, srcDest);
> +          releaseScratch(scr);
> +      } else
> +          m_assembler.xorl_i8r(imm.m_value, srcDest);

Please use 4 space intention.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:393
> +

Please remove this empty line.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:684
> +        return branchTrue();
> +

Please remove this empty line. Or move it before the return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:745
> +        if (left != dest) {
> +            m_assembler.loadConstant(right.m_value, dest);
> +            set32Compare32(cond, left, dest, dest);
> +        } else {
> +            RegisterID scr = claimScratch();
> +            m_assembler.loadConstant(right.m_value, scr);
> +            set32Compare32(cond, left, scr, dest);
> +            releaseScratch(scr);
> +        }

We usually prefer early return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:771
> +
> +
> +

No comment for this "section"?

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:858
> +            m_assembler.movt(dest);

Early return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:871
> +           move(right, dest);
> +           set32Compare32(cond, left, dest, dest);

Early return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:892
> +            m_assembler.movt(dest);

Early return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:962
> +

Empty line

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1256
> +        if ((cond == Zero) || (cond == NonZero)) {

Early return

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1413
> +    private:

No indentation

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1422
> +    static void linkCall(void* code, Call call, FunctionPtr function);
> +
> +    static void repatchCall(CodeLocationCall call, CodeLocationLabel destination);
> +
> +    static void repatchCall(CodeLocationCall call, FunctionPtr destination);
> +

No need for empty lines

> Source/JavaScriptCore/assembler/SH4Assembler.cpp:4
> +/*
> + *  SH4Assembler.cpp
> + *  Copyright (C) 2011 STMicroelectronics. All rights reserved.
> +*/

License header!

> Source/JavaScriptCore/assembler/SH4Assembler.h:50
> +    return ((opc) | (((rm) & 0xf) << 8) |(((rn) & 0xf) << 4));

Please use correct spaceing and remove the unneeded braces.
For the other functions in this file too!

> Source/JavaScriptCore/assembler/SH4Assembler.h:438
> +         ASSERT((claimscratchReg != 0x3));
> +         RegisterID scratchReg = scratchReg1;
> +
> +         if (!(claimscratchReg & 0x1))
> +             claimscratchReg = (claimscratchReg | 0x1);
> +         else {
> +             claimscratchReg = (claimscratchReg | 0x2);
> +             scratchReg = scratchReg2;
> +         }
> +         return scratchReg;
> +     }
> +
> +     void releaseScratch(RegisterID scratchR) 
> +     {   
> +         if (scratchR ==  scratchReg1)
> +             claimscratchReg = (claimscratchReg & 0x2);
> +         else
> +             claimscratchReg = (claimscratchReg & 0x1);
> +     } 

4 space indentation

> Source/JavaScriptCore/assembler/SH4Assembler.h:477
> +           RegisterID scr = claimScratch(); 

4 space indentation

> Source/JavaScriptCore/assembler/SH4Assembler.h:574
> +

Empty line

> Source/JavaScriptCore/assembler/SH4Assembler.h:581
> +

Empty line

> Source/JavaScriptCore/assembler/SH4Assembler.h:594
> +

Empty line

> Source/JavaScriptCore/assembler/SH4Assembler.h:629
> +

Empty line

> Source/JavaScriptCore/assembler/SH4Assembler.h:882
> +            uint16_t opc = getOpcodeGroup5(TSTIMM_OPCODE, imm);
> +            oneShortOp(opc);

Early return

> Source/JavaScriptCore/assembler/SH4Assembler.h:2078
> +    private:
> +        SH4Buffer m_buffer;

No indentation

[thouraya]

Created attachment 84424 [details]
YARR JIT 

Hi,

many thanks for the review.

I applied the changes you suggested for the code style and submitted a new patch.

Regards,
Thouraya.

[Patrick R. Gansterer]

Comment on attachment 84424 [details]
YARR JIT 

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

Style improved a lot, but still some issues. Most of them are valid for whole code.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.cpp:32
> +

No need for this empty line

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:31
> +#include "assert.h"
> +#include <wtf/Platform.h>

I don't think this headers are required.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:32
> +#if ENABLE(ASSEMBLER)

Use ENABLE(ASSEMBLER) && CPU(SH4)

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:42
> +

Empty line

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:363
> +    void load32(RegisterID base, int offset, RegisterID dest)

You can use early return in this function too.
There are many similar if/else constructs which should use early return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:815
> +       m_assembler.loadConstant(imm.m_value, dest);

4 space indentation

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:822
> +       DataLabelPtr dataLabel(this);
> +       m_assembler.loadConstant(reinterpret_cast<uint32_t>(initialValue.m_value), dest, true);
> +       return dataLabel;

4 space indentation

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1424
> +    static void linkCall(void*, Call, FunctionPtr);
> +
> +    static void repatchCall(CodeLocationCall, CodeLocationLabel);
> +
> +    static void repatchCall(CodeLocationCall, FunctionPtr);

Empty lines

> Source/JavaScriptCore/assembler/SH4Assembler.cpp:36
> +#ifdef SH4_ASSEMBLER_TRACING
> +bool SH4Assembler::dumpNativeCode = false;
> +#endif

I don't see a ASSEMBLER_TRACING for the other cpus. So I'm not sure if we want this code at all.
If want the tracing code in the tree, I'd prefer to get rid of this variable and move all code into a bool dumpNativeCode() in the header.
Using a own file for this variable is overkill IMHO.

> Source/JavaScriptCore/assembler/SH4Assembler.h:30
> +#include <wtf/Platform.h>

IMHO no need for this header

> Source/JavaScriptCore/assembler/SH4Assembler.h:42
> +#ifndef va_list
> +#include <stdarg.h>
> +#endif

Can't we add stdarg.h all the time?

> Source/JavaScriptCore/assembler/SH4Assembler.h:48
> +inline uint16_t getOpcodeGroup1(uint16_t opc, int rm, int rn)

Why are these function not in the JSC namespace?

> Source/JavaScriptCore/assembler/SH4Assembler.h:365
> +        } Condition;

I don't think this enum needs so much indentation. ;-)

> Source/JavaScriptCore/assembler/SH4Assembler.h:649
> +        case 1: opc = getOpcodeGroup2(SHLL_OPCODE, dst);
> +            break;

Please move the getOpcodeGroup2 and ASSERT_NOT_REACHED calls into their own lines.

> Source/JavaScriptCore/assembler/SH4Assembler.h:951
> +        default: ASSERT_NOT_REACHED();

New line for ASSERT_NOT_REACHED

> Source/JavaScriptCore/assembler/SH4Assembler.h:1101
> +    // double operations

You usually have an empty line after the "section header"

> Source/JavaScriptCore/assembler/SH4Assembler.h:1154
> +             fmovReadr0r(base, dest);

Early return

> Source/JavaScriptCore/assembler/SH4Assembler.h:1229
> +    // Various move ops:

You don't use a colon at the other "section headers". Please add it to all other or remove it everywhere

> Source/JavaScriptCore/assembler/SH4Assembler.h:1373
> +

empty line

> Source/JavaScriptCore/assembler/SH4Assembler.h:1658
> +         }

4 space indentation

> Source/JavaScriptCore/assembler/SH4Assembler.h:1711
> +            *addr = offsetBits;

early return

[Oliver Hunt]

(In reply to comment #80)
> (From update of attachment 84424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84424&action=review
> 
> Style improved a lot, but still some issues. Most of them are valid for whole code.
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerSH4.cpp:32
> > +
> 
> No need for this empty line
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:31
> > +#include "assert.h"

Indeed, assert.h is just wrong -- you should be using <wtf/Assertions.h>

[ssseintr]

Hi thouraya,

Thanks for your SH4 JIT Patch.

Please point me to get the latest SH4 JIT patch against any latest Webkit revision. From STLinux website I can able to get up-to  webkit revsion 76915. But now Webkit much advanced(80210).

Thanks & Regards,
Vicky

[thouraya]

Created attachment 84728 [details]
YARR JIT 

Hello,

Attached a new patch containing some code cleanup.

Many thanks for the review.

regards,
Thouraya.

[ssseintr]

Hi thouraya,

Pls send me the procedure to update your patch for nightly builds downloaded from webkit.org. I have Webkit-r74228.

Thanks & Regards,
Vicky.

[thouraya]

Hello,

Do you have any objection regarding the patch?


Regards,
Thouraya.

[Gavin Barraclough]

The yarr patch looks like it's in good shape to me, to be honest I'd be pretty hard pushed to even nit-pick.

I'm happy to r+ as is, but given the large change I'm going to give other reviewers a brief chance to comment before doing so.

[thouraya]

Created attachment 85674 [details]
YAR JIT + Floating point support

Hello,

Updated the YARR patch to the top of the trunk and added floating point support.

Regards,
Thouraya.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:36:  __fpscr_values is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 7 files


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

[Eduardo Felipe Castegnaro]

(In reply to comment #87)
> Updated the YARR patch to the top of the trunk and added floating point support.

Thouraya,

With your latest patch I'm getting the following error:

Source/JavaScriptCore/jit/JITStubs.h:233:2: error: #error "JITStackFrame not defined for this platform."

When checking your code it doesn't define a #CPU(SH4) conditional for JITStackFrame, while other supported CPUs like x86, Mips and ARM do.

Is there anything I should do so JITStackFrame is defined for SH4?

Thanks in advance,

Eduardo.

[thouraya]

Hi,

The patch you applied contains only YARR support (not all JIT).

Regards,
Thouraya.

[thouraya]

Created attachment 86940 [details]
YARR  for SH4

[thouraya]

Created attachment 86941 [details]
JIT part for sh4

[thouraya]

Created attachment 86942 [details]
Enable JIT build for sh4

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:258:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:429:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 8 files


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

[thouraya]

Hello,

I submitted 3 patchs as suggested:
- the first to add YARR support.
- the second to add the remaining JIT part.
- the last one to enable JIT.

Regards,
Thouraya.

[thouraya]

Created attachment 87170 [details]
YARR for SH4

Hello,

merged last YARR patch with https://bug-46796-attachments.webkit.org/attachment.cgi?id=86911 patch as requested by Gabor in Bug 46796.

Regards,
Thouraya.

[thouraya]

Hello,

Any feedback concerning the patches?

Best regards,
Thouraya.

[Oliver Hunt]

Comment on attachment 87170 [details]
YARR for SH4

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

[Gabor Loki]

> > Source/JavaScriptCore/assembler/AssemblerBuffer.h:-131
> > -        template<typename IntegralType>
> > -        void putIntegral(IntegralType value)
> 
> 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?
> 
> I think these should stay in the root assembler buffer so we don't have unnecessary changes in this patch

Well, I asked Thouraya to move 'putIntegral.*TwoShorts' functions from this patch https://bug-46796-attachments.webkit.org/attachment.cgi?id=86911 to AssemblerBufferWithConstantPool level. But I did not mean to move all 'putIntegral.*IntegralType' template functions to AssemblerBufferWithConstantPool. So, my suggestion was to move only 'void putIntegral(TwoShorts value)' and 'void putIntegralUnchecked(TwoShorts value)' functions to AssemblerBufferWithConstantPool.

[thouraya]

Created attachment 87681 [details]
YARR for SH4

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 8 files


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

[thouraya]

Created attachment 87693 [details]
YARR for SH4

Updated YARR JIT to the top of the trunk.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 8 files


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

[thouraya]

Hello,

I did  such a changes because I saw that the putIntegral was just called in AssemblerBufferWithConstantPool.h file.

Now, I removed my changes an submitted a new patch but has got the error related to the style.

Is the patch OK?
Should I fix the style issue ?

Many thanks for the review.

Regards,
Thouraya.

(In reply to comment #99)
> > > Source/JavaScriptCore/assembler/AssemblerBuffer.h:-131
> > > -        template<typename IntegralType>
> > > -        void putIntegral(IntegralType value)
> > 
> > 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?
> > 
> > I think these should stay in the root assembler buffer so we don't have unnecessary changes in this patch
> 
> Well, I asked Thouraya to move 'putIntegral.*TwoShorts' functions from this patch https://bug-46796-attachments.webkit.org/attachment.cgi?id=86911 to AssemblerBufferWithConstantPool level. But I did not mean to move all 'putIntegral.*IntegralType' template functions to AssemblerBufferWithConstantPool. So, my suggestion was to move only 'void putIntegral(TwoShorts value)' and 'void putIntegralUnchecked(TwoShorts value)' functions to AssemblerBufferWithConstantPool.

[Gabor Loki]

> Should I fix the style issue ?
> ...
>     https://bugs.webkit.org/show_bug.cgi?id=44329.

I guess the dot causes that style issue in your Changelog.

> I did  such a changes because I saw that the putIntegral was just called in AssemblerBufferWithConstantPool.h file.

Although the putIntegral template functions is used only in AssemblerBufferWithConstantPool, it would be nice if we can replace all putByte, -Short, -Int, -Int64 functions with the putIntegral template. These task is not related to SH4 JIT. It should be done in a separate patch.

[thouraya]

Created attachment 87714 [details]
YARR for SH4

removed the dot to fix style issue.

[Oliver Hunt]

Comment on attachment 87714 [details]
YARR for SH4

r=me

[WebKit Commit Bot]

Comment on attachment 87714 [details]
YARR for SH4

Clearing flags on attachment: 87714

Committed r82617: <http://trac.webkit.org/changeset/82617>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82617 might have broken Qt Linux Release

[thouraya]

Hello,

> http://trac.webkit.org/changeset/82617 might have broken Qt Linux Release

Does the patch caused that problem? or it's a warning ?

Regards,
Thouraya.

[Adam Barth]

> > http://trac.webkit.org/changeset/82617 might have broken Qt Linux Release
> 
> Does the patch caused that problem? or it's a warning ?

Oftentimes the bots get behind and test a bunch of changes together.  That means the automatic tools can't figure out which patch caused the problem.  In this case, it wasn't your patch.  It was another one.

[thouraya]

Created attachment 88415 [details]
JIT remaining part for SH4 platforms.

Hello,

Here attached the remaining JIT part for SH4 platform.


Regards,
Thouraya.

[thouraya]

Hello,

Any feedback concerning the JIT remaining part for SH4 platforms patch?

Best Regards,
Thouraya.

[Oliver Hunt]

Comment on attachment 88415 [details]
JIT remaining part for SH4 platforms.

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

r- due to the END_UNINTERRUPTED_SEQUENCE stuff

> Source/JavaScriptCore/assembler/SH4Assembler.h:36
> +#include <stdio.h>

this seems unnecessary

> Source/JavaScriptCore/jit/JIT.h:712
> +#define END_UNINTERRUPTED_SEQUENCES(name, extraI, extraC) endUninterruptedSequence(name ## InstructionSpace + extraI, name ## ConstantSpace + extraC)

macros like this should always be wrapped in a do {} while (false) construct to reduce the chance of incorrect use.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:485
> +#if CPU(SH4)
> +    if ((dst >= 0) && (dst <= 7))
> +        END_UNINTERRUPTED_SEQUENCE(sequenceGetByIdSlowCase);
> +    else if ((dst > 15) || (dst < -16))
> +        END_UNINTERRUPTED_SEQUENCES(sequenceGetByIdSlowCase, 8, 2);
> +    else
> +        END_UNINTERRUPTED_SEQUENCES(sequenceGetByIdSlowCase, 8, 0);
> +#else

Rather than having this ifdef here you should have this logic in the platform specific handler for uninterrupted sequence.  If absolutely necessary you can just make END_UNINTERRUPTED_SEQUENCE takes a dst parameter.

[thouraya]

Created attachment 88819 [details]
JIT remaining part for SH4 platforms.

Hello,

I the attached patch, I added END_UNINTERRUPTED_SEQUENCE2 macro that takes 2 parameters name and dst and 
modified END_UNINTERRUPTED_SEQUENCE to call END_UNINTERRUPTED_SEQUENCE2(name, 0)

Regards,
Thouraya.

[Early Warning System Bot]

Attachment 88819 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8373536

[Build Bot]

Attachment 88819 [details] did not build on win:
Build output: http://queues.webkit.org/results/8379002

[thouraya]

Created attachment 88821 [details]
JIT remaining part for SH4 platforms.

[thouraya]

Created attachment 88826 [details]
Enable JIT

[thouraya]

Hello,

The changes done for END_UNINTERRUPTED_SEQUENCE are OK ?

Best Regards,
Thouraya.

[Oliver Hunt]

Comment on attachment 88821 [details]
JIT remaining part for SH4 platforms.

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

r- due to naming issue, almost there.

> Source/JavaScriptCore/jit/JIT.h:710
> +#define END_UNINTERRUPTED_SEQUENCE2(name, dst) do { endUninterruptedSequence(name ## InstructionSpace, name ## ConstantSpace, dst); } while (false)

This is a bad name, if anything END_UNINTERRUPTED_SEQUENCE_FOR_PUT might be better?

> Source/JavaScriptCore/jit/JITInlineMethods.h:143
> +ALWAYS_INLINE void JIT::endUninterruptedSequence(int insnSpace, int constSpace, int dst)

Does the arm build work with this change?  afaict it should fail due to dst being unused.

You could put UNUSED_PARAM(dst) in the function to deal with that.

[thouraya]

Created attachment 88983 [details]
JIT remaining part for SH4 platforms.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/jit/JITInlineMethods.h:143:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 9 files


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

[thouraya]

Created attachment 88986 [details]
JIT remaining part for SH4 platforms.

[WebKit Commit Bot]

Comment on attachment 88986 [details]
JIT remaining part for SH4 platforms.

Clearing flags on attachment: 88986

Committed r83447: <http://trac.webkit.org/changeset/83447>

[WebKit Commit Bot]

Comment on attachment 88826 [details]
Enable JIT 

Clearing flags on attachment: 88826

Committed r83495: <http://trac.webkit.org/changeset/83495>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[STEPHAN Gael]

I may be wrong (again), but not all patches landed in the trunk ( looks like YARR for SH4 did not make it)

[thouraya]

(In reply to comment #128)
> I may be wrong (again), but not all patches landed in the trunk ( looks like YARR for SH4 did not make it)

Hello,

All patches were landed.

Regards,
Thouraya.