Bug 231362 – [JSC][32bit] Fix wrong branchAdd32 assembly for ARMv7

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mikhail R. Gadelha]

[JSC][32bit] Fix wrong branchAdd32 assembly for ARMv7

[Mikhail R. Gadelha]

Created attachment 440502 [details]
Patch

[Keith Miller]

Comment on attachment 440502 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1859
> +        // Do the add.
> +        ARMThumbImmediate armImm = ARMThumbImmediate::makeEncodedImm(imm.m_value);
> +        if (armImm.isValid())
> +            m_assembler.add_S(dataTempRegister, dataTempRegister, armImm);
> +        else {
> +            move(imm, addressTempRegister);
> +            m_assembler.add_S(dataTempRegister, dataTempRegister, addressTempRegister);
> +        }

Can we add an add32 with flags helper that is this? It seems like that could be useful elsewhere in this file.

[Mikhail R. Gadelha]

Created attachment 440530 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 440530 [details]
Patch

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

r=me with changes.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:200
> +    void add32(TrustedImm32 imm, Address address, bool updateFlags = false)

Let's define it as add32Impl helper private function, and define add32 as

void add32(TrustedImm32 imm, Address address)
{
    constexpr bool updateFlags = false;
    add32Impl(imm, address, updateFlags);
}

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:229
> +    void add32(TrustedImm32 imm, AbsoluteAddress address, bool updateFlags = false)

Let's define it as add32Impl helper private function, and define add32 as

void add32(TrustedImm32 imm, AbsoluteAddress address)
{
    constexpr bool updateFlags = false;
    add32Impl(imm, address, updateFlags);
}

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1861
> +        add32(imm, dest, true);

Let's make it readable by,

constexpr bool updateFlags = true;
add32(imm, dest, updateFlags);

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1867
> +        add32(imm, dest, true);

Let's make it readable by,

constexpr bool updateFlags = true;
add32(imm, dest, updateFlags);

[Mikhail R. Gadelha]

Created attachment 441077 [details]
Patch

[EWS]

Committed r284103 (242931@main): <https://commits.webkit.org/242931@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441077 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84206141>