Bug 104103 - Implement add64 for ARM traditional assembler after r136601
Summary: Implement add64 for ARM traditional assembler after r136601
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 104085 (view as bug list)
Depends on:
Blocks: 102999
  Show dependency treegraph
 
Reported: 2012-12-05 02:02 PST by Csaba Osztrogonác
Modified: 2012-12-12 00:01 PST (History)
11 users (show)

See Also:


Attachments
Fix (1.48 KB, patch)
2012-12-05 02:29 PST, Gabor Ballabas
zherczeg: review-
zherczeg: commit-queue-
Details | Formatted Diff | Diff
Patch (2.36 KB, patch)
2012-12-05 07:12 PST, Gabor Ballabas
zherczeg: review-
zherczeg: commit-queue-
Details | Formatted Diff | Diff
Patch (2.56 KB, patch)
2012-12-11 04:33 PST, Gabor Ballabas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-12-05 02:02:12 PST
r136601 broke the ARM traditional build:

/mnt/raptor2/slaves/qt5-linux-armv7-release/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp: In member function 'void JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node&)':
/mnt/raptor2/slaves/qt5-linux-armv7-release/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:5048:15: error: 'class JSC::DFG::JITCompiler' has no member named 'add64'

It seems ARM traditional assembler needs add64.
Comment 1 Gabor Ballabas 2012-12-05 02:29:15 PST
Created attachment 177706 [details]
Fix
Comment 2 Zoltan Herczeg 2012-12-05 02:33:17 PST
Comment on attachment 177706 [details]
Fix

good direction, but it is not finished.

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

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:127
> +        move(TrustedImmPtr(address.m_ptr), ARMRegisters::S0);
> +        load32(Address(ARMRegisters::S0), ARMRegisters::S1);
> +        add32(imm, ARMRegisters::S1);
> +        store32(ARMRegisters::S1, ARMRegisters::S0);

This is actually the add32 version. You need to take care about the higher 32 bit with addc/subc. And you should use the adds directly (I know add32 now uses adds, but that may be go away. I mean I want it to go away :) )
Comment 3 Gabor Ballabas 2012-12-05 07:12:12 PST
Created attachment 177742 [details]
Patch
Comment 4 Zoltan Herczeg 2012-12-06 04:02:06 PST
Comment on attachment 177742 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:905
> +        if (imm.m_value <= 0xff)
> +            m_assembler.adds(ARMRegisters::S0, ARMRegisters::S0, ARMAssembler::getOp2Byte(imm.m_value));
> +        else {
> +            m_assembler.adds(ARMRegisters::S0, ARMRegisters::S0, m_assembler.getImm(imm.m_value, ARMRegisters::S1));
> +            move(TrustedImmPtr(address.m_ptr), ARMRegisters::S1);
> +        }

This is not how we do at the moment. This should be a simple:

m_assembler.adds(dest, dest, m_assembler.getImm(imm.m_value, ARMRegisters::S0));
Comment 5 Gabor Ballabas 2012-12-11 04:33:02 PST
Created attachment 178778 [details]
Patch
Comment 6 Vivek Galatage 2012-12-11 19:37:03 PST
*** Bug 104085 has been marked as a duplicate of this bug. ***
Comment 7 Zoltan Herczeg 2012-12-11 23:51:41 PST
Comment on attachment 178778 [details]
Patch

r=me
Comment 8 WebKit Review Bot 2012-12-12 00:01:34 PST
Comment on attachment 178778 [details]
Patch

Clearing flags on attachment: 178778

Committed r137426: <http://trac.webkit.org/changeset/137426>
Comment 9 WebKit Review Bot 2012-12-12 00:01:40 PST
All reviewed patches have been landed.  Closing bug.