Re: RFC: [PATCH,ARM] Fix 56110
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?
Re: RFC: [PATCH,ARM] Fix 56110
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 [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 [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 [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?
RFC: [PATCH,ARM] Fix 56110
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
[PATCH,testsuite] Added test for PR 55987
Hi,
here's a patch that adds a testcase for PR 55987.
Is xfail the right thing to use here? I went with that since I guess the
PR won't be fixed anytime soon ;)
I haven't assigned copyright to the FSF -- is this patch small enough to
go in without it?
Thanks,
Tilman
2013-02-14 Tilman Sauerbeck
PR target/55987
* gcc.target/arm/pr55987.c: New.
diff --git a/gcc/testsuite/gcc.target/arm/pr55987.c
b/gcc/testsuite/gcc.target/arm/pr55987.c
new file mode 100644
index 000..7dd9de0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr55987.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler-times ".word" 1 { xfail *-*-* } } } */
+/* { dg-final { scan-assembler "bic" { xfail *-*-* } } } */
+
+unsigned f(unsigned x, unsigned y)
+{
+ unsigned c = 0x7f7f7f7f;
+
+ return (x & c) ^ (y & ~c);
+}
