RFC: [PATCH,ARM] Fix 56110

Richard Earnshaw [2013-02-19 15:12]:

Hi,
thanks for your reply.

> [...]
> However, the question you need to be asking is why the pattern immediately
> before the one you've added is not matching.  The compiler knows how to add
> clobbers, so I'm surprised that you're finding a new pattern to be
> necessary.

From the andsi3_compare0_scratch pattern definition:
> [(set (reg:CC_NOOV CC_REGNUM)
>        (compare:CC_NOOV
>         (and:SI (match_operand:SI 0 "s_register_operand" "r,r,r")
>                 (match_operand:SI 1 "arm_not_operand" "I,K,r"))
                                       ^^^^^^^^^^^^^^^^
>         (const_int 0)))

So the insn pattern only matches if the 2nd operand is an
arm_not_operand.  In my test program, the 2nd operand is 0x8080
(const_int 32896 [0x8080]) which cannot be used as an immediate operand
(const_ok_for_arm doesn't match 0x8080 nor ~0x8080).

I don't see _why_ we would want the pattern to only be applied
to registers or immediates though.

Actually the diff below makes it so that gcc recognizes the pattern even if
0x8080 is used as the 2nd operand. It looks like using
reg_or_int_operand should be the correct thing to do, since we do have
the I/K constraints in the pattern...

However it breaks the case where the 2nd operand is a const_int that
*can* be used as an immediate (eg 0x80), and ends up generating the
AND/CMP combination.

For the latter case, RTL looks like this before the combiner pass:

> (insn 7 4 8 2 (set (reg:SI 114 [ D.4127 ])
>         (and:SI (reg/v:SI 113 [ m ])
>             (const_int 128 [0x80]))) /home/user/g.c:3 76 {*arm_andsi3_insn}
>      (expr_list:REG_DEAD (reg/v:SI 113 [ m ])
>         (nil)))
> (insn 8 7 9 2 (set (reg:CC 100 cc)
>         (compare:CC (reg:SI 114 [ D.4127 ])
>             (const_int 0 [0]))) /home/user/g.c:3 217 {*arm_cmpsi_insn}
>      (expr_list:REG_DEAD (reg:SI 114 [ D.4127 ])
>         (nil)))
 
In the combiner pass:
 
> Successfully matched this instruction:
> (set (reg:SI 114 [ D.4127 ])
>     (and:SI (reg:SI 1 r1 [ m ])
>         (const_int 128 [0x80])))
> [...]
>
> Failed to match this instruction:
> (set (reg:CC_NOOV 100 cc)
>     (compare:CC_NOOV (zero_extract:SI (reg:SI 1 r1 [ m ])
>             (const_int 1 [0x1])
>             (const_int 7 [0x7]))
>         (const_int 0 [0])))

I don't get why relaxing the restrictions for the
andsi3_compare0_scratch pattern results in a mismatch for the
zeroextractsi_compare0_scratch one.

Any ideas?

Thanks,
Tilman

Tilman Sauerbeck [2013-02-19 23:26]:

> However it breaks the case where the 2nd operand is a const_int that
> *can* be used as an immediate (eg 0x80), and ends up generating the
> AND/CMP combination.

... and that would be because I changed the operand patterns in
zeroextractsi_compare0_scratch and didn't restore the originals when
trying the new diff :/

So the diff I posted last night might do the trick.
If there's no obvious reason why it would be wrong, I'll start a
bootstrap and regression test tonight.

Thanks,
Tilman

On 19/02/13 22:26, Tilman Sauerbeck wrote:
> I don't get why relaxing the restrictions for the
> andsi3_compare0_scratch pattern results in a mismatch for the
> zeroextractsi_compare0_scratch one.
>
> Any ideas?

Because of the way combine works.  It first tries to find a pattern that 
doesn't have a clobber expression.  It finds your new pattern and then 
uses that.  But since that can't handle immediates, reload then comes 
along and forces the constant into a register.

You need one pattern to deal with all the cases.

R.

Richard Earnshaw [2013-02-20 11:00]:
> On 19/02/13 22:26, Tilman Sauerbeck wrote:
> >I don't get why relaxing the restrictions for the
> >andsi3_compare0_scratch pattern results in a mismatch for the
> >zeroextractsi_compare0_scratch one.
> >
> >Any ideas?
> 
> Because of the way combine works.  It first tries to find a pattern that
> doesn't have a clobber expression.  It finds your new pattern and then uses
> that.  But since that can't handle immediates, reload then comes along and
> forces the constant into a register.
> 
> You need one pattern to deal with all the cases.

You mean the pattern should include calls to arm_split_constant() to do
the loading itself, like e.g. the iorsi3 pattern does?
Why can't we let reload do the load?

FWIW the patch in http://gcc.gnu.org/ml/gcc-patches/2013-02/msg00920.html
works for my testcases, survives a bootstrap in qemu and passes the
test suite (I only built/tested the C and C++ frontends though).

Thanks,
Tilman

Tilman Sauerbeck [2013-02-24 17:00]:
> Richard Earnshaw [2013-02-20 11:00]:
> > On 19/02/13 22:26, Tilman Sauerbeck wrote:
> > >I don't get why relaxing the restrictions for the
> > >andsi3_compare0_scratch pattern results in a mismatch for the
> > >zeroextractsi_compare0_scratch one.
> > >
> > >Any ideas?
> > 
> > Because of the way combine works.  It first tries to find a pattern that
> > doesn't have a clobber expression.  It finds your new pattern and then uses
> > that.  But since that can't handle immediates, reload then comes along and
> > forces the constant into a register.
> > 
> > You need one pattern to deal with all the cases.
> 
> You mean the pattern should include calls to arm_split_constant() to do
> the loading itself, like e.g. the iorsi3 pattern does?
> Why can't we let reload do the load?
> 
> FWIW the patch in http://gcc.gnu.org/ml/gcc-patches/2013-02/msg00920.html
> works for my testcases, survives a bootstrap in qemu and passes the
> test suite (I only built/tested the C and C++ frontends though).

Sorry to be a pain, but ... ping?
I don't know how to proceed with this patch.

Thanks.

Regards,
Tilman