Bug 172972 - [ARMv6][DFG] ARM MacroAssembler is always emitting cmn when immediate is 0
Summary: [ARMv6][DFG] ARM MacroAssembler is always emitting cmn when immediate is 0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords:
: 154853 154856 (view as bug list)
Depends on:
Blocks: 108645 172765
  Show dependency treegraph
 
Reported: 2017-06-06 06:01 PDT by Caio Lima
Modified: 2017-06-20 02:09 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.34 KB, patch)
2017-06-13 20:29 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (3.00 KB, patch)
2017-06-14 19:30 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (2.99 KB, patch)
2017-06-15 08:30 PDT, Caio Lima
mark.lam: review+
mark.lam: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (2.83 KB, patch)
2017-06-19 06:01 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (988.64 KB, application/zip)
2017-06-19 07:01 PDT, Build Bot
no flags Details
Patch for landing (2.84 KB, patch)
2017-06-19 07:51 PDT, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2017-06-06 06:01:27 PDT
...
Comment 1 Caio Lima 2017-06-13 18:51:19 PDT
Actually, the real problem here is because Comparisons into ARMv6 MacroAssembler are being emitted incorrectly. Patch coming soon.
Comment 2 Caio Lima 2017-06-13 20:29:23 PDT
Created attachment 312851 [details]
Patch
Comment 3 Mark Lam 2017-06-14 11:05:17 PDT
Comment on attachment 312851 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        semantics of cmn is oposite of cmp[1]. One case that it's breaking is

typo: /oposite/opposite/

> Source/JavaScriptCore/ChangeLog:21
> +        when $r0 > 0. In that case, the correct opcode is "cmp". Whith this

typo: /Whith/With/

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1609
> -        if (tmp != ARMAssembler::InvalidImmediate)
> -            m_assembler.cmn(left, tmp);
> -        else
> +        if (tmp != ARMAssembler::InvalidImmediate) {
> +            if (isUInt12(right.m_value))
> +                m_assembler.cmp(left, tmp);
> +            else
> +                m_assembler.cmn(left, tmp);
> +        } else

This is wrong; you'll turn a "cmn r0, #1" into a "cmp r0, #1", which is plainly wrong.

tmp is an encoding of -right.m_value.  Hence, the correct comparison operation to use is always cmn, with one exception: 0.  That is because -0 is still 0, and hence, the sign polarity is not inverted as required for cmn to work properly.  You can fix the issue by first checking for 0 before doing the cmn, or you can solve this in a more generic way like so:

    ARMWord tmp = m_assembler.getOp2(right.m_value);
    if (tmp != ARMAssembler::InvalidImmediate) {
        m_assembler.cmp(left, tmp);
        return;
    }
    ASSERT(right.m_value != 0 && static_cast<unsigned>(right.m_value) != 0x80000000);
    tmp = m_assembler.getOp2(-right.m_value);
    if (tmp != ARMAssembler::InvalidImmediate) {
        m_assembler.cmn(left, tmp);
        return;
    }
    m_assembler.cmp(left, m_assembler.getImm(right.m_value, ARMRegisters::S0));

The m_assembler.getOp2(right.m_value) case is guaranteed to take care of 0 and 0x80000000 (and a lot of other possible constants as well).  Hence, it is now safe to try cmn on -right.m_value.

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1622
> -        if (tmp != ARMAssembler::InvalidImmediate)
> -            m_assembler.cmn(ARMRegisters::S1, tmp);
> -        else
> +        if (tmp != ARMAssembler::InvalidImmediate) {
> +            if (isUInt12(right.m_value))
> +                m_assembler.cmp(ARMRegisters::S1, tmp);
> +            else
> +                m_assembler.cmn(ARMRegisters::S1, tmp);
> +        } else

Ditto.  This is wrong.
Comment 4 Mark Lam 2017-06-14 11:12:17 PDT
Comment on attachment 312851 [details]
Patch

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

>> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1609
>> +        } else
> 
> This is wrong; you'll turn a "cmn r0, #1" into a "cmp r0, #1", which is plainly wrong.
> 
> tmp is an encoding of -right.m_value.  Hence, the correct comparison operation to use is always cmn, with one exception: 0.  That is because -0 is still 0, and hence, the sign polarity is not inverted as required for cmn to work properly.  You can fix the issue by first checking for 0 before doing the cmn, or you can solve this in a more generic way like so:
> 
>     ARMWord tmp = m_assembler.getOp2(right.m_value);
>     if (tmp != ARMAssembler::InvalidImmediate) {
>         m_assembler.cmp(left, tmp);
>         return;
>     }
>     ASSERT(right.m_value != 0 && static_cast<unsigned>(right.m_value) != 0x80000000);
>     tmp = m_assembler.getOp2(-right.m_value);
>     if (tmp != ARMAssembler::InvalidImmediate) {
>         m_assembler.cmn(left, tmp);
>         return;
>     }
>     m_assembler.cmp(left, m_assembler.getImm(right.m_value, ARMRegisters::S0));
> 
> The m_assembler.getOp2(right.m_value) case is guaranteed to take care of 0 and 0x80000000 (and a lot of other possible constants as well).  Hence, it is now safe to try cmn on -right.m_value.

Looks like m_assembler.getImm() already tries to encode m_assembler.getOp2(right.m_value).  Hence, the fix is actually to add the !right.m_value check only like so:
 
    ARMWord tmp = (!right.m_value || static_cast<unsigned>(right.m_value) == 0x80000000) ? ARMAssembler::InvalidImmediate : m_assembler.getOp2(-right.m_value);
    if (tmp != ARMAssembler::InvalidImmediate)
        m_assembler.cmn(left, tmp);
    else
        m_assembler.cmp(left, m_assembler.getImm(right.m_value, ARMRegisters::S0));
Comment 5 Caio Lima 2017-06-14 19:30:00 PDT
Created attachment 312946 [details]
Patch
Comment 6 Caio Lima 2017-06-14 19:35:51 PDT
(In reply to Mark Lam from comment #4)
> Comment on attachment 312851 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312851&action=review
> 
> >> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1609
> >> +        } else
> > 
> > This is wrong; you'll turn a "cmn r0, #1" into a "cmp r0, #1", which is plainly wrong.
> > 
> > tmp is an encoding of -right.m_value.  Hence, the correct comparison operation to use is always cmn, with one exception: 0.  That is because -0 is still 0, and hence, the sign polarity is not inverted as required for cmn to work properly.  You can fix the issue by first checking for 0 before doing the cmn, or you can solve this in a more generic way like so:

Oh, I think that I understood it wrongly. According my debug code here, all positive values fallback into cmp branch, since "m_assembler.getOp2" results in ARMAssembler::InvalidImmediate for them.
 
> Looks like m_assembler.getImm() already tries to encode
> m_assembler.getOp2(right.m_value).  Hence, the fix is actually to add the
> !right.m_value check only like so:
>  
>     ARMWord tmp = (!right.m_value || static_cast<unsigned>(right.m_value) ==
> 0x80000000) ? ARMAssembler::InvalidImmediate :
> m_assembler.getOp2(-right.m_value);
>     if (tmp != ARMAssembler::InvalidImmediate)
>         m_assembler.cmn(left, tmp);
>     else
>         m_assembler.cmp(left, m_assembler.getImm(right.m_value,
> ARMRegisters::S0));

Yes. I agree that it's the right fix. I think that just #0 is broken here. Running javascript-core locally to check failures.

Thank you for the quick review.
Comment 7 Caio Lima 2017-06-15 08:30:19 PDT
Created attachment 312984 [details]
Patch
Comment 8 Mark Lam 2017-06-15 09:55:23 PDT
Comment on attachment 312984 [details]
Patch

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

r=me.  Please fix the typo in the ChangeLog.

> Source/JavaScriptCore/ChangeLog:11
> +        semantics of cmn is oposite of cmp[1]. One case that it's breaking is

please fix typo: /oposite/opposite/
Comment 9 Caio Lima 2017-06-19 06:01:57 PDT
Created attachment 313282 [details]
Patch for landing

ChangeLog updated.
Comment 10 Build Bot 2017-06-19 07:01:34 PDT
Comment on attachment 313282 [details]
Patch for landing

Attachment 313282 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3957395

New failing tests:
inspector/canvas/create-canvas-contexts.html
Comment 11 Build Bot 2017-06-19 07:01:35 PDT
Created attachment 313290 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Caio Lima 2017-06-19 07:51:06 PDT
Created attachment 313296 [details]
Patch for landing

Rebased with master
Comment 13 WebKit Commit Bot 2017-06-19 17:50:26 PDT
Comment on attachment 313296 [details]
Patch for landing

Clearing flags on attachment: 313296

Committed r218519: <http://trac.webkit.org/changeset/218519>
Comment 14 WebKit Commit Bot 2017-06-19 17:50:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Csaba Osztrogonác 2017-06-20 02:07:55 PDT
*** Bug 154853 has been marked as a duplicate of this bug. ***
Comment 16 Csaba Osztrogonác 2017-06-20 02:08:01 PDT
*** Bug 154856 has been marked as a duplicate of this bug. ***
Comment 17 Csaba Osztrogonác 2017-06-20 02:09:41 PDT
And it already fixed failing rest parameters and spread tests too.