[Webkit-unassigned] [Bug 44329] SH4 JIT SUPPORT

Fri Feb 11 07:02:54 PST 2011

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

--- Comment #60 from thouraya <thouraya.andolsi at st.com>  2011-02-11 07:02:54 PST ---
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 ?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.