* [PATCH] cmpelim: recognize extra clobbers in insns
@ 2020-08-06 12:42 Pip Cet
2020-08-19 11:05 ` Richard Sandiford
2020-08-24 18:11 ` Jeff Law
0 siblings, 2 replies; 6+ messages in thread
From: Pip Cet @ 2020-08-06 12:42 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 775 bytes --]
I'm working on the AVR cc0 -> CCmode conversion (bug#92729). One
problem is that the cmpelim pass is currently very strict in requiring
insns of the form
(parallel [(set (reg:SI) (op:SI ... ...))
(clobber (reg:CC REG_CC))])
when in fact AVR's insns often have the form
(parallel [(set (reg:SI) (op:SI ... ...))
(clobber (scratch:QI))
(clobber (reg:CC REG_CC))])
The attached patch relaxes checks in the cmpelim code to recognize
such insns, and makes it attempt to recognize
(parallel [(set (reg:CC REG_CC) (compare:CC ... ...))
(set (reg:SI (op:SI ... ...)))
(clobber (scratch:QI))])
as a new insn for that example. This appears to work.
I've bootstrapped and run the test suite with the patch, without differences.
[-- Attachment #2: 0001-cmpelim-handle-extra-clobbers.patch --]
[-- Type: application/x-patch, Size: 2965 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cmpelim: recognize extra clobbers in insns
2020-08-06 12:42 [PATCH] cmpelim: recognize extra clobbers in insns Pip Cet
@ 2020-08-19 11:05 ` Richard Sandiford
2020-08-20 11:59 ` Pip Cet
2020-08-24 18:11 ` Jeff Law
1 sibling, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2020-08-19 11:05 UTC (permalink / raw)
To: Pip Cet via Gcc-patches
Sorry for the slow reply.
Pip Cet via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> I'm working on the AVR cc0 -> CCmode conversion (bug#92729). One
> problem is that the cmpelim pass is currently very strict in requiring
> insns of the form
>
> (parallel [(set (reg:SI) (op:SI ... ...))
> (clobber (reg:CC REG_CC))])
>
> when in fact AVR's insns often have the form
>
> (parallel [(set (reg:SI) (op:SI ... ...))
> (clobber (scratch:QI))
> (clobber (reg:CC REG_CC))])
>
> The attached patch relaxes checks in the cmpelim code to recognize
> such insns, and makes it attempt to recognize
>
> (parallel [(set (reg:CC REG_CC) (compare:CC ... ...))
> (set (reg:SI (op:SI ... ...)))
> (clobber (scratch:QI))])
>
> as a new insn for that example. This appears to work.
The idea looks good. However, I think it'd be better (or at least
more usual) for the define_insns to list the clobbers the other
way around:
(parallel [(set (reg:SI) (op:SI ... ...))
(clobber (reg:CC REG_CC))
(clobber (scratch:QI))])
(clobber (scratch…))s generally come last because any rtl optimisation
pass that uses recog can automatically add any necessary
(clobber (scratch…))s. In contrast, very few passes (probably just
combine) know how to add (clobber (reg:CC REG_CC)) to a pattern that
didn't already have it. This is because adding a REG_CC clobber
requires the pass to “prove” that REG_CC is dead at that point,
whereas there are no restrictions on adding a scratch clobber.
Thanks,
Richard
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cmpelim: recognize extra clobbers in insns
2020-08-19 11:05 ` Richard Sandiford
@ 2020-08-20 11:59 ` Pip Cet
2020-08-21 10:05 ` Richard Sandiford
0 siblings, 1 reply; 6+ messages in thread
From: Pip Cet @ 2020-08-20 11:59 UTC (permalink / raw)
To: Pip Cet via Gcc-patches, Pip Cet, richard.sandiford
On Wed, Aug 19, 2020 at 11:05 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
> Sorry for the slow reply.
Not a problem at all. Thank you for responding!
> Pip Cet via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > I'm working on the AVR cc0 -> CCmode conversion (bug#92729). One
> > problem is that the cmpelim pass is currently very strict in requiring
> > insns of the form
> >
> > (parallel [(set (reg:SI) (op:SI ... ...))
> > (clobber (reg:CC REG_CC))])
> >
> > when in fact AVR's insns often have the form
> >
> > (parallel [(set (reg:SI) (op:SI ... ...))
> > (clobber (scratch:QI))
> > (clobber (reg:CC REG_CC))])
> >
> > The attached patch relaxes checks in the cmpelim code to recognize
> > such insns, and makes it attempt to recognize
> >
> > (parallel [(set (reg:CC REG_CC) (compare:CC ... ...))
> > (set (reg:SI (op:SI ... ...)))
> > (clobber (scratch:QI))])
> >
> > as a new insn for that example. This appears to work.
>
> The idea looks good. However, I think it'd be better (or at least
> more usual) for the define_insns to list the clobbers the other
> way around:
>
> (parallel [(set (reg:SI) (op:SI ... ...))
> (clobber (reg:CC REG_CC))
> (clobber (scratch:QI))])
That makes sense, thanks for the suggestion. I realized quite quickly
that I would have to reproduce the patterns precisely, including order
in a parallel, and decided to go with the wrong consistent option by
always placing the CC clobber last.
My question is whether it makes more sense to recognize either form
(i.e. a clobber of targetm.flags_regnum at any position in a parallel)
or whether it's okay to assume that the clobber is always the second
element in the parallel. I'm leaning towards the latter version.
> (clobber (scratch…))s generally come last because any rtl optimisation
> pass that uses recog can automatically add any necessary
> (clobber (scratch…))s. In contrast, very few passes (probably just
> combine) know how to add (clobber (reg:CC REG_CC)) to a pattern that
> didn't already have it. This is because adding a REG_CC clobber
> requires the pass to “prove” that REG_CC is dead at that point,
> whereas there are no restrictions on adding a scratch clobber.
Thanks again!
Pip
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cmpelim: recognize extra clobbers in insns
2020-08-20 11:59 ` Pip Cet
@ 2020-08-21 10:05 ` Richard Sandiford
0 siblings, 0 replies; 6+ messages in thread
From: Richard Sandiford @ 2020-08-21 10:05 UTC (permalink / raw)
To: Pip Cet; +Cc: Pip Cet via Gcc-patches
Pip Cet <pipcet@gmail.com> writes:
>> Pip Cet via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> > I'm working on the AVR cc0 -> CCmode conversion (bug#92729). One
>> > problem is that the cmpelim pass is currently very strict in requiring
>> > insns of the form
>> >
>> > (parallel [(set (reg:SI) (op:SI ... ...))
>> > (clobber (reg:CC REG_CC))])
>> >
>> > when in fact AVR's insns often have the form
>> >
>> > (parallel [(set (reg:SI) (op:SI ... ...))
>> > (clobber (scratch:QI))
>> > (clobber (reg:CC REG_CC))])
>> >
>> > The attached patch relaxes checks in the cmpelim code to recognize
>> > such insns, and makes it attempt to recognize
>> >
>> > (parallel [(set (reg:CC REG_CC) (compare:CC ... ...))
>> > (set (reg:SI (op:SI ... ...)))
>> > (clobber (scratch:QI))])
>> >
>> > as a new insn for that example. This appears to work.
>>
>> The idea looks good. However, I think it'd be better (or at least
>> more usual) for the define_insns to list the clobbers the other
>> way around:
>>
>> (parallel [(set (reg:SI) (op:SI ... ...))
>> (clobber (reg:CC REG_CC))
>> (clobber (scratch:QI))])
>
> That makes sense, thanks for the suggestion. I realized quite quickly
> that I would have to reproduce the patterns precisely, including order
> in a parallel, and decided to go with the wrong consistent option by
> always placing the CC clobber last.
>
> My question is whether it makes more sense to recognize either form
> (i.e. a clobber of targetm.flags_regnum at any position in a parallel)
> or whether it's okay to assume that the clobber is always the second
> element in the parallel. I'm leaning towards the latter version.
Yeah, I agree the latter sounds best. We should be able to test whether
the rest of the parallel is suitable by adding a single_set test
(after testing everything else).
Thanks,
Richard
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cmpelim: recognize extra clobbers in insns
2020-08-06 12:42 [PATCH] cmpelim: recognize extra clobbers in insns Pip Cet
2020-08-19 11:05 ` Richard Sandiford
@ 2020-08-24 18:11 ` Jeff Law
2020-08-28 6:16 ` Pip Cet
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Law @ 2020-08-24 18:11 UTC (permalink / raw)
To: Pip Cet, gcc-patches
On Thu, 2020-08-06 at 12:42 +0000, Pip Cet via Gcc-patches wrote:
> I'm working on the AVR cc0 -> CCmode conversion (bug#92729). One
> problem is that the cmpelim pass is currently very strict in requiring
> insns of the form
>
> (parallel [(set (reg:SI) (op:SI ... ...))
> (clobber (reg:CC REG_CC))])
>
> when in fact AVR's insns often have the form
>
> (parallel [(set (reg:SI) (op:SI ... ...))
> (clobber (scratch:QI))
> (clobber (reg:CC REG_CC))])
>
> The attached patch relaxes checks in the cmpelim code to recognize
> such insns, and makes it attempt to recognize
>
> (parallel [(set (reg:CC REG_CC) (compare:CC ... ...))
> (set (reg:SI (op:SI ... ...)))
> (clobber (scratch:QI))])
>
> as a new insn for that example. This appears to work.
>
> I've bootstrapped and run the test suite with the patch, without differences.
So it looks like Richard has given you some feedback and you've got some further
work to do. I just wanted to chime in and say thanks for tackling this.
The H8 has similiar problems -- I've fixed them in a different way, but improving
cmp-elim is still a good thing.
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cmpelim: recognize extra clobbers in insns
2020-08-24 18:11 ` Jeff Law
@ 2020-08-28 6:16 ` Pip Cet
0 siblings, 0 replies; 6+ messages in thread
From: Pip Cet @ 2020-08-28 6:16 UTC (permalink / raw)
To: law; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 737 bytes --]
On Mon, Aug 24, 2020 at 6:12 PM Jeff Law <law@redhat.com> wrote:
> On Thu, 2020-08-06 at 12:42 +0000, Pip Cet via Gcc-patches wrote:
> > I've bootstrapped and run the test suite with the patch, without differences.
> So it looks like Richard has given you some feedback and you've got some further
> work to do. I just wanted to chime in and say thanks for tackling this.
Thanks for the encouragement!
Richard wrote:
> We should be able to test whether the rest of the parallel is suitable by adding a single_set test (after testing everything else).
It seems to me the existing two checks using single_set are sufficient
to ensure this.
Revised patch attached. It bootstraps and causes no test regressions,
and it appears to work.
[-- Attachment #2: 0001-cmpelim-handle-extra-clobbers.patch --]
[-- Type: text/x-patch, Size: 2502 bytes --]
From 72e05ac3a570f3e6ca9e54599db1b3b4daa84e90 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Tue, 25 Aug 2020 14:23:58 +0000
Subject: [PATCH] cmpelim: handle extra clobbers
Handle extra clobbers in CC-clobbering insns when attempting to
recognize the corresponding CC-setting insn.
This is for the AVR CCmode conversion. AVR has insns like
(define_insn "andhi3"
[(set (match_operand:HI 0 ...)
(and:HI (match_operand:HI 1 ...)
(match_operand:HI 2 ...)))
(clobber (reg:CC REG_CC))
(clobber (match_scratch:QI 3 ...))] ...)
which can be profitably converted into CC-setting variants.
2020-08-28 Pip Cet <pipcet@gmail.com>
gcc/ChangeLog:
* compare-elim.c (arithmetic_flags_clobber_p): Allow extra
clobbers. (try_validate_parallel): (try_eliminate_compare):
Likewise.
---
gcc/compare-elim.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/gcc/compare-elim.c b/gcc/compare-elim.c
index 85f3e344074..8e5c9a05fa8 100644
--- a/gcc/compare-elim.c
+++ b/gcc/compare-elim.c
@@ -202,7 +202,7 @@ arithmetic_flags_clobber_p (rtx_insn *insn)
if (asm_noperands (pat) >= 0)
return false;
- if (GET_CODE (pat) == PARALLEL && XVECLEN (pat, 0) == 2)
+ if (GET_CODE (pat) == PARALLEL && XVECLEN (pat, 0) >= 2)
{
x = XVECEXP (pat, 0, 0);
if (GET_CODE (x) != SET)
@@ -663,6 +663,17 @@ static rtx
try_validate_parallel (rtx set_a, rtx set_b)
{
rtx par = gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, set_a, set_b));
+ if (GET_CODE (set_b) == PARALLEL)
+ {
+ int len = XVECLEN (set_b, 0);
+ rtvec v = rtvec_alloc (len);
+ RTVEC_ELT (v, 0) = set_a;
+ RTVEC_ELT (v, 1) = XVECEXP (set_b, 0, 0);
+ for (int i = 2; i < len; i++)
+ RTVEC_ELT (v, i) = XVECEXP (set_b, 0, i);
+
+ par = gen_rtx_PARALLEL (VOIDmode, v);
+ }
rtx_insn *insn = make_insn_raw (par);
if (insn_invalid_p (insn, false))
@@ -873,10 +884,13 @@ try_eliminate_compare (struct comparison *cmp)
[(set (reg:CCM) (compare:CCM (operation) (immediate)))
(set (reg) (operation)] */
- rtvec v = rtvec_alloc (2);
+ int len = XVECLEN (PATTERN (insn), 0);
+ rtvec v = rtvec_alloc (len);
RTVEC_ELT (v, 0) = y;
RTVEC_ELT (v, 1) = x;
-
+ for (int i = 2; i < len; i++)
+ RTVEC_ELT (v, i) = XVECEXP (PATTERN (insn), 0, i);
+
rtx pat = gen_rtx_PARALLEL (VOIDmode, v);
/* Succeed if the new instruction is valid. Note that we may have started
--
2.28.0
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-28 6:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-06 12:42 [PATCH] cmpelim: recognize extra clobbers in insns Pip Cet
2020-08-19 11:05 ` Richard Sandiford
2020-08-20 11:59 ` Pip Cet
2020-08-21 10:05 ` Richard Sandiford
2020-08-24 18:11 ` Jeff Law
2020-08-28 6:16 ` Pip Cet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).