Bug 68301 - Macro assembler branch8 & 16 methods vary in treatment of upper bits
Summary: Macro assembler branch8 & 16 methods vary in treatment of upper bits
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-16 18:29 PDT by Michael Saboff
Modified: 2011-09-27 16:34 PDT (History)
1 user (show)

See Also:


Attachments
Fix for branch16 (17.58 KB, patch)
2011-09-24 00:29 PDT, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2011-09-16 18:29:02 PDT
The martial register macro assembler functions vary in their handling of the "unused" bits.  Some of the functions generate code that ignores the unused bits, either via anding with a mask or shifting.  Other functions assume that the unused bits are zero.  Below are some examples of each.

There are two ways we can proceed, either fix functions that don't ignore to ignore the unused bits or change the convention so that a 32 bit compare will work in all cases.  For architectures that can perform zero filled (unsigned extend) loads of 8 and 16 bit quantities, the later solution would be fine.  

Function that generates code to ignore unused bits:

From MacroAssemblerARM.h:
    Jump branch16(RelationalCondition cond, RegisterID left, TrustedImm32 right)
    {
        ASSERT(!(right.m_value & 0xFFFF0000));
        right.m_value <<= 16;
        m_assembler.mov_r(ARMRegisters::S1, left);
        lshift32(TrustedImm32(16), ARMRegisters::S1);
        return branch32(cond, ARMRegisters::S1, right);
    }

From MacroAssemblerARMv7.h:
    Jump branch16(RelationalCondition cond, BaseIndex left, TrustedImm32 right)
    {
        // use addressTempRegister incase the branch32 we call uses dataTempRegister. :-/
        load16(left, addressTempRegister);
        m_assembler.lsl(addressTempRegister, addressTempRegister, 16);
        return branch32(cond, addressTempRegister, TrustedImm32(right.m_value << 16));
    }

Function that assumes unused bits are zero:

From MacroAssemblerArmv7.h:
    Jump branch8(RelationalCondition cond, RegisterID left, TrustedImm32 right)
    {
        compare32(left, right);
        return Jump(makeBranch(cond));
    }

From MacroAssemblerMIPS.h:
    Jump branch16(RelationalCondition cond, BaseIndex left, RegisterID right)
    {
        load16(left, dataTempRegister);
        return branch32(cond, dataTempRegister, right);
    }
Comment 1 Gavin Barraclough 2011-09-16 19:07:16 PDT
In the case of branch8/16, the current intended behaviour is for any bits other than the low N to be ignored (where N is 8/16).  Any MacroAssembler implementation that does otherwise may function incorrectly.

This is easy to implement correctly on x86, but may be expensive on other platforms.  In many cases we may be able to change the JIT to ensure the full 32-bit value is known (e.g. zero extended), and call branch32 instead.

If we can change all client code to use branch32 we can fix this by simply removing the 8/16 bit compares.
Comment 2 Gavin Barraclough 2011-09-24 00:29:53 PDT
Created attachment 108579 [details]
Fix for branch16
Comment 3 Gavin Barraclough 2011-09-27 16:34:28 PDT
Fixed (removed) branch16 in r96169, but still need to check correctness of branch8.