RFC: [PATCH,ARM] Fix 56110

RFC: [PATCH,ARM] Fix 56110

[Tilman Sauerbeck]

Hi,
adding the instruction pattern below fixes my testcase for PR 56110;
however I'm not sure if adding a new pattern is the correct way to go.

I duplicated the andsi3_compare0_scratch pattern, and lifted the
requirement that the 2nd operand be an arm_not_operand. I didn't copy
over the clobber pattern because I don't know what it does ;)

Comments? Can anyone put me in the right direction here? Or take my
humble attempt and massage it into the proper fix?
I can provide a dejagnu testcase if it helps.

No ChangeLog entry because I know this diff won't go anywhere in its
current incarnation.

Thanks,
Tilman

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 64888f9..e47f8f7 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -2212,6 +2212,19 @@
    (set_attr "type"  "simple_alu_imm,simple_alu_imm,*")]
 )
 
+(define_insn "*andsi3_compare0_scratch2"
+  [(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 "const_int_operand" "r,r,r"))
+	 (const_int 0)))]
+  "TARGET_32BIT"
+  "@
+   tst%?\\t%0, %1"
+  [(set_attr "conds" "set")
+   (set_attr "type"  "simple_alu_imm,simple_alu_imm,*")]
+)
+
 (define_insn "*zeroextractsi_compare0_scratch"
   [(set (reg:CC_NOOV CC_REGNUM)
 	(compare:CC_NOOV (zero_extract:SI

Re: RFC: [PATCH,ARM] Fix 56110

[Tilman Sauerbeck]

Tilman Sauerbeck [2013-02-18 22:47]:

> [...]
> +  "TARGET_32BIT"
> +  "@
> +   tst%?\\t%0, %1"
> [...]

I managed to post the wrong diff -- line 2 in the citation should be
omitted of course.  Sorry.

Regards,
Tilman

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

Re: RFC: [PATCH,ARM] Fix 56110

[Richard Earnshaw]

On 18/02/13 21:47, Tilman Sauerbeck wrote:
> Hi,
> adding the instruction pattern below fixes my testcase for PR 56110;
> however I'm not sure if adding a new pattern is the correct way to go.
>
> I duplicated the andsi3_compare0_scratch pattern, and lifted the
> requirement that the 2nd operand be an arm_not_operand. I didn't copy
> over the clobber pattern because I don't know what it does ;)
>
> Comments? Can anyone put me in the right direction here? Or take my
> humble attempt and massage it into the proper fix?
> I can provide a dejagnu testcase if it helps.
>
> No ChangeLog entry because I know this diff won't go anywhere in its
> current incarnation.
>
> Thanks,
> Tilman
>
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 64888f9..e47f8f7 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -2212,6 +2212,19 @@
>      (set_attr "type"  "simple_alu_imm,simple_alu_imm,*")]
>   )
>
> +(define_insn "*andsi3_compare0_scratch2"
> +  [(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 "const_int_operand" "r,r,r"))
> +	 (const_int 0)))]
> +  "TARGET_32BIT"
> +  "@
> +   tst%?\\t%0, %1"
> +  [(set_attr "conds" "set")
> +   (set_attr "type"  "simple_alu_imm,simple_alu_imm,*")]
> +)
> +
>   (define_insn "*zeroextractsi_compare0_scratch"
>     [(set (reg:CC_NOOV CC_REGNUM)
>   	(compare:CC_NOOV (zero_extract:SI
>



Sorry, this is not correct.  You've got a constraint that requires a 
const_int_operand, but then specified only a register.  This will force 
the compiler to reload the immediate operand into a register, just 
negating any saving you've made from getting rid of the compare 
instruction.  Then you've added three identical variants all using two 
registers of the same class.

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.

R.

Re: RFC: [PATCH,ARM] Fix 56110

[Tilman Sauerbeck]

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


diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 64888f9..3724a8d 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -2200,7 +2200,7 @@
   [(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"))
+                (match_operand:SI 1 "reg_or_int_operand" "I,K,r"))
         (const_int 0)))
    (clobber (match_scratch:SI 2 "=X,r,X"))]
   "TARGET_32BIT"

Re: RFC: [PATCH,ARM] Fix 56110

[Tilman Sauerbeck]

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

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

Re: RFC: [PATCH,ARM] Fix 56110

[Richard Earnshaw]

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.

Re: RFC: [PATCH,ARM] Fix 56110

[Tilman Sauerbeck]

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

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

Re: RFC: [PATCH,ARM] Fix 56110

[Tilman Sauerbeck]

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

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?