[webkit-reviews] review denied: [Bug 172972] [ARMv6][DFG] ARM MacroAssembler is always emitting comparisons with cmn and it breaks conditional branches like bhi. : [Attachment 312851] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 172972: [ARMv6][DFG] ARM MacroAssembler is always emitting comparisons with
cmn and it breaks conditional branches like bhi.
https://bugs.webkit.org/show_bug.cgi?id=172972

Attachment 312851: Patch

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




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 312851
  --> https://bugs.webkit.org/attachment.cgi?id=312851
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.