Clobber REG_CC only for some constraint alternatives?

Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj]

Hi,

  I'm working on porting AVR to MODE_CC, and there are quite a few
  patterns that clobber the condition code reg only for certain
  constraint alternatives. For e.g.,

(define_insn "mov<mode>_insn"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,d    ,Qm   ,r ,q,r,*r")
        (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r Y00,Qm,r,q,i"))]
 "register_operand (operands[0], <MODE>mode)
 || reg_or_0_operand (operands[1], <MODE>mode)"
 {
      return output_movqi (insn, operands, NULL);
 }
 [(set_attr "length" "1,1,5,5,1,1,4")
 (set_attr "adjust_len" "mov8")
 (set_attr "cc" "ldi,none,clobber,clobber,none,none,clobber")])

As you can deduce from the (set_attr "cc" ..), only constraint
alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.

My first version of the port adds a post-reload splitter that adds a
(clobber (reg:CC REG_CC)) unconditionally, and it appears to work. If I
do want to add the clobber conditionally, would something like the below
be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner way?

(define_insn_and_split "mov<mode>_insn"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,d    ,Qm   ,r ,q,r,*r")
        (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r Y00,Qm,r,q,i"))]
  "register_operand (operands[0], <MODE>mode)
    || reg_or_0_operand (operands[1], <MODE>mode)"
  "#"
  "&& reload_completed"
  [(parallel [(set (match_dup 0)
                   (match_dup 1))
              (clobber (match_dup 2))])]
  {
    operands[2] = get_cc_reg_clobber_rtx (curr_insn);
  }
  [(set_attr "cc" "ldi,none,clobber,clobber,none,none,clobber")])

(define_insn "*mov<mode>_insn_clobber_flags"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,Qm   ,r ,*r")
        (match_operand:ALL1 1 "nox_general_operand"   "r Y00,r Y00,Qm,i"))
   (clobber (reg:CC REG_CC))]
  "(register_operand (operands[0], <MODE>mode)
    || reg_or_0_operand (operands[1], <MODE>mode))
   && reload_completed"
  {
    return output_movqi (insn, operands, NULL);
  }
  [(set_attr "length" "1,5,5,4")
   (set_attr "adjust_len" "mov8")])

(define_insn "*mov<mode>_insn_noclobber_flags"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d   ,q,r")
        (match_operand:ALL1 1 "nox_general_operand"   "r,n Ynn,r,q"))
   (clobber (const_int 0))]
  "(register_operand (operands[0], <MODE>mode)
    || reg_or_0_operand (operands[1], <MODE>mode))
   && reload_completed"
  {
    return output_movqi (insn, operands, NULL);
  }
  [(set_attr "length" "1,1,1,1")
   (set_attr "adjust_len" "mov8")])

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Matt Wette]

On 8/14/20 4:16 AM, Senthil Kumar Selvaraj via Gcc wrote:
> Hi,
>
>    I'm working on porting AVR to MODE_CC, and there are quite a few
>    patterns that clobber the condition code reg only for certain
>    constraint alternatives. For e.g.,
>
> (define_insn "mov<mode>_insn"
>    [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,d    ,Qm   ,r ,q,r,*r")
>          (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r Y00,Qm,r,q,i"))]
>   "register_operand (operands[0], <MODE>mode)
>   || reg_or_0_operand (operands[1], <MODE>mode)"
>   {
>        return output_movqi (insn, operands, NULL);
>   }
>   [(set_attr "length" "1,1,5,5,1,1,4")
>   (set_attr "adjust_len" "mov8")
>   (set_attr "cc" "ldi,none,clobber,clobber,none,none,clobber")])
>
> As you can deduce from the (set_attr "cc" ..), only constraint
> alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.
>
> My first version of the port adds a post-reload splitter that adds a
> (clobber (reg:CC REG_CC)) unconditionally, and it appears to work. If I
> do want to add the clobber conditionally, would something like the below
> be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
> or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner way?
>
> (define_insn_and_split "mov<mode>_insn"
>    [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,d    ,Qm   ,r ,q,r,*r")
>          (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r Y00,Qm,r,q,i"))]
>    "register_operand (operands[0], <MODE>mode)
>      || reg_or_0_operand (operands[1], <MODE>mode)"
>    "#"
>    "&& reload_completed"
>    [(parallel [(set (match_dup 0)
>                     (match_dup 1))
>                (clobber (match_dup 2))])]
>    {
>      operands[2] = get_cc_reg_clobber_rtx (curr_insn);
>    }
>    [(set_attr "cc" "ldi,none,clobber,clobber,none,none,clobber")])
>
> (define_insn "*mov<mode>_insn_clobber_flags"
>    [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,Qm   ,r ,*r")
>          (match_operand:ALL1 1 "nox_general_operand"   "r Y00,r Y00,Qm,i"))
>     (clobber (reg:CC REG_CC))]
>    "(register_operand (operands[0], <MODE>mode)
>      || reg_or_0_operand (operands[1], <MODE>mode))
>     && reload_completed"
>    {
>      return output_movqi (insn, operands, NULL);
>    }
>    [(set_attr "length" "1,5,5,4")
>     (set_attr "adjust_len" "mov8")])
>
> (define_insn "*mov<mode>_insn_noclobber_flags"
>    [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d   ,q,r")
>          (match_operand:ALL1 1 "nox_general_operand"   "r,n Ynn,r,q"))
>     (clobber (const_int 0))]
>    "(register_operand (operands[0], <MODE>mode)
>      || reg_or_0_operand (operands[1], <MODE>mode))
>     && reload_completed"
>    {
>      return output_movqi (insn, operands, NULL);
>    }
>    [(set_attr "length" "1,1,1,1")
>     (set_attr "adjust_len" "mov8")])
>
> Regards
> Senthil

Happy to see someone working this.   Are you starting with one CC mode?
I noticed that the current CC0 implementation seems to effectively use
several modes.  For example, one for use of the t flag.  I'm sure it 
will be easier
to start with one mode.

Matt

Re: Clobber REG_CC only for some constraint alternatives?

[Pip Cet]

On Fri, Aug 14, 2020 at 3:33 PM Matt Wette via Gcc <gcc@gcc.gnu.org> wrote:
> Happy to see someone working this.   Are you starting with one CC mode?

I'm also working on this (mostly at bug#92792), and so far am using
two modes: the general reg:CC mode for proper comparison insns, and
CCNZ for optimization in the CC-setting variants produced by the
cmpelim pass.

> I noticed that the current CC0 implementation seems to effectively use
> several modes.  For example, one for use of the t flag.

As far as I can tell, the current code emits T setters and getters
together as one insn, even going to the trouble of setting T
redundantly when it already has been set. Am I missing something?

Re: Clobber REG_CC only for some constraint alternatives?

[Segher Boessenkool]

Hi!

On Fri, Aug 14, 2020 at 04:46:59PM +0530, Senthil Kumar Selvaraj via Gcc wrote:
>   I'm working on porting AVR to MODE_CC,

I'm very happy to see people work on that.

> (define_insn "*mov<mode>_insn_noclobber_flags"
>   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d   ,q,r")
>         (match_operand:ALL1 1 "nox_general_operand"   "r,n Ynn,r,q"))
>    (clobber (const_int 0))]

This is not correct, clobbers like that are not defined RTL, and are
actually used internally (by combine at least), so this will confuse
that.

If you want to say some alternative does not clobber anything, just use
the constraint "X" for that alternative.

$ grep -r "clobber.*scratch.*X" gcc/config/

HtH,


Segher

Re: Clobber REG_CC only for some constraint alternatives?

[Pip Cet]

On Fri, Aug 14, 2020 at 4:24 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Fri, Aug 14, 2020 at 04:46:59PM +0530, Senthil Kumar Selvaraj via Gcc wrote:
> > (define_insn "*mov<mode>_insn_noclobber_flags"
> >   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d   ,q,r")
> >         (match_operand:ALL1 1 "nox_general_operand"   "r,n Ynn,r,q"))
> >    (clobber (const_int 0))]
>
> This is not correct, clobbers like that are not defined RTL, and are
> actually used internally (by combine at least), so this will confuse
> that.
>
> If you want to say some alternative does not clobber anything, just use
> the constraint "X" for that alternative.

Just to clarify, such clobbers would still show up in RTL, right?
Because some passes, such as cmpelim, do not currently appear to deal
very well with extra clobbers, so that might be a problem.

What I'm currently doing is this:

(define_split
  [(set (match_operand 0 "nonimmediate_operand")
    (match_operand 1 "nox_general_operand"))]
  "reload_completed && mov_clobbers_cc (insn, operands)"
  [(parallel [(set (match_dup 0) (match_dup 1))
          (clobber (reg:CC REG_CC))])])

which, if I understand correctly, introduces the parallel-clobber
expression only when necessary, at the same time as post-reload
splitters introduce REG_CC references. Is that correct?

Thanks
Pip

Re: Clobber REG_CC only for some constraint alternatives?

[Segher Boessenkool]

Hi!

On Fri, Aug 14, 2020 at 05:47:02PM +0000, Pip Cet wrote:
> On Fri, Aug 14, 2020 at 4:24 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Fri, Aug 14, 2020 at 04:46:59PM +0530, Senthil Kumar Selvaraj via Gcc wrote:
> > > (define_insn "*mov<mode>_insn_noclobber_flags"
> > >   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d   ,q,r")
> > >         (match_operand:ALL1 1 "nox_general_operand"   "r,n Ynn,r,q"))
> > >    (clobber (const_int 0))]
> >
> > This is not correct, clobbers like that are not defined RTL, and are
> > actually used internally (by combine at least), so this will confuse
> > that.
> >
> > If you want to say some alternative does not clobber anything, just use
> > the constraint "X" for that alternative.
> 
> Just to clarify, such clobbers would still show up in RTL, right?

Yes, as

  (clobber (scratch:CC))

(or whatever the mode is).  No register will be allocated to it.  You
can do a define_split splitting it into the form without clobber, if
you want?  (You can "split" to just one insn fine.)  It's neatest when
written as a define_insn_and_split.

> Because some passes, such as cmpelim, do not currently appear to deal
> very well with extra clobbers, so that might be a problem.

Not sure if it would mind here...  It is not a clobber of a reg.

> What I'm currently doing is this:
> 
> (define_split
>   [(set (match_operand 0 "nonimmediate_operand")
>     (match_operand 1 "nox_general_operand"))]
>   "reload_completed && mov_clobbers_cc (insn, operands)"
>   [(parallel [(set (match_dup 0) (match_dup 1))
>           (clobber (reg:CC REG_CC))])])
> 
> which, if I understand correctly, introduces the parallel-clobber
> expression only when necessary, at the same time as post-reload
> splitters introduce REG_CC references. Is that correct?

Yes.  And this will work correctly if somehow you ensure that REG_CC
isn't live anywhere you introduce such a clobber.


Segher

Re: Clobber REG_CC only for some constraint alternatives?

[Pip Cet]

On Sat, Aug 15, 2020 at 12:30 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Fri, Aug 14, 2020 at 05:47:02PM +0000, Pip Cet wrote:
> > On Fri, Aug 14, 2020 at 4:24 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > If you want to say some alternative does not clobber anything, just use
> > > the constraint "X" for that alternative.
> >
> > Just to clarify, such clobbers would still show up in RTL, right?
>
> Yes, as
>
>   (clobber (scratch:CC))
>
> (or whatever the mode is).  No register will be allocated to it.  You
> can do a define_split splitting it into the form without clobber, if
> you want?  (You can "split" to just one insn fine.)  It's neatest when
> written as a define_insn_and_split.

That does sound like a neat solution for leaving the current patterns
largely intact, thanks! I'll try both variants, both and without the
define_split.

> > What I'm currently doing is this:
> >
> > (define_split
> >   [(set (match_operand 0 "nonimmediate_operand")
> >     (match_operand 1 "nox_general_operand"))]
> >   "reload_completed && mov_clobbers_cc (insn, operands)"
> >   [(parallel [(set (match_dup 0) (match_dup 1))
> >           (clobber (reg:CC REG_CC))])])
> >
> > which, if I understand correctly, introduces the parallel-clobber
> > expression only when necessary, at the same time as post-reload
> > splitters introduce REG_CC references. Is that correct?
>
> Yes.  And this will work correctly if somehow you ensure that REG_CC
> isn't live anywhere you introduce such a clobber.

IIUC, the idea is that references to REG_CC, except for clobbers, are
only introduced in the post-reload split pass, so it cannot be live
before our define_split runs. Does that make sense?

Re: Clobber REG_CC only for some constraint alternatives?

[Segher Boessenkool]

On Sat, Aug 15, 2020 at 10:18:27AM +0000, Pip Cet wrote:
> > > What I'm currently doing is this:
> > >
> > > (define_split
> > >   [(set (match_operand 0 "nonimmediate_operand")
> > >     (match_operand 1 "nox_general_operand"))]
> > >   "reload_completed && mov_clobbers_cc (insn, operands)"
> > >   [(parallel [(set (match_dup 0) (match_dup 1))
> > >           (clobber (reg:CC REG_CC))])])
> > >
> > > which, if I understand correctly, introduces the parallel-clobber
> > > expression only when necessary, at the same time as post-reload
> > > splitters introduce REG_CC references. Is that correct?
> >
> > Yes.  And this will work correctly if somehow you ensure that REG_CC
> > isn't live anywhere you introduce such a clobber.
> 
> IIUC, the idea is that references to REG_CC, except for clobbers, are
> only introduced in the post-reload split pass, so it cannot be live
> before our define_split runs. Does that make sense?

Yes, it does.  It has some huge restrictions (using the reg in inline
assembler can never work reliably, for example, so you'll have to
disallow that).  And earlier passes (like combine) cannot optimise this,
etc.  But it is a pretty straightforward way to move from CC0 to the
modern world!


Segher

Re: Clobber REG_CC only for some constraint alternatives?

[Pip Cet]

On Sun, Aug 16, 2020 at 12:50 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Sat, Aug 15, 2020 at 10:18:27AM +0000, Pip Cet wrote:
> > > > What I'm currently doing is this:
> > > >
> > > > (define_split
> > > >   [(set (match_operand 0 "nonimmediate_operand")
> > > >     (match_operand 1 "nox_general_operand"))]
> > > >   "reload_completed && mov_clobbers_cc (insn, operands)"
> > > >   [(parallel [(set (match_dup 0) (match_dup 1))
> > > >           (clobber (reg:CC REG_CC))])])
> > > >
> > > > which, if I understand correctly, introduces the parallel-clobber
> > > > expression only when necessary, at the same time as post-reload
> > > > splitters introduce REG_CC references. Is that correct?
> > >
> > > Yes.  And this will work correctly if somehow you ensure that REG_CC
> > > isn't live anywhere you introduce such a clobber.
> >
> > IIUC, the idea is that references to REG_CC, except for clobbers, are
> > only introduced in the post-reload split pass, so it cannot be live
> > before our define_split runs. Does that make sense?
>
> Yes, it does.  It has some huge restrictions (using the reg in inline
> assembler can never work reliably, for example, so you'll have to
> disallow that).

Is there any approach that doesn't suffer from that problem? My
understanding was that we need to allow reload to insert CC-clobbering
insns on this (and many other) architectures, and that there are so
many places where reload might choose to do so (including before and
after inline asm) that using the register prior to reload just isn't
possible. I'd be glad to be wrong, though :-)

Is it true that reload chooses which constraint alternative is used
for each insn? Is that information available somehow to post-reload
splits? The problem is that the "X" constraint matches whatever's
there already, and doesn't replace it with the (scratch:CC) expression
that would work, so I can't rewrite those insns not to clobber the CC
reg.

For example, here's what I currently have:

(define_expand "mov<mode>"
  [(parallel [(set (match_operand:MOVMODE 0 "nonimmediate_operand" "")
           (match_operand:MOVMODE 1 "general_operand" ""))
          (clobber (reg:CC REG_CC))])]
  ...)

(define_insn "mov<mode>_insn"
   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,r  ,d    ,Qm
  ,r ,q,r,*r")
        (match_operand:ALL1 1 "nox_general_operand"   "r,Y00,n Ynn,r
Y00,Qm,r,q,i"))
   (clobber (match_scratch:CC 2 "=X,c,X,c,c,X,X,c"))]
  ...)

That works, but it results in an incorrect CC clobber for, say,
register-to-register movqi. For that, I'd need something like

(define_split
  [(parallel [(set (match_operand:ALL1 0 "nonimmediate_operand")
             (match_operand:ALL1 1 "nox_general_operand"))
          (clobber (reg:CC REG_CC))])]
  "reload_completed && REG_P (operands[0]) && REG_P (operands[1])"
  [(parallel [(set (match_dup 0)
           (match_dup 1))
          (clobber (scratch:CC))])])

and so on, for all four constraint alternatives that don't actually
clobber REG_CC (and also for a fifth which only rarely clobbers
REG_CC). That's duplicated code, of course.

All this is at least somewhat academic: the code produced isn't
drastically better after my cc conversion, but it's not usually any
worse, and I'm still looking at assembler examples that are pessimized
a little to find out how to fix them...

> And earlier passes (like combine) cannot optimise this,

I'm not sure what "this" refers to: my understanding is that the idea
is to let combine see CC-free code, or code with CC clobbers only,
which it can then optimize, and only add "real" CC references after
reload, so many optimizations should just work.

> But it is a pretty straightforward way to move from CC0 to the
> modern world!

With the limitations of the reload pass being as they are, I don't
really see a dramatically better alternative. I suppose we could
explicitly save and restore CC flags around insns emitted when
reload_in_progress is true, to simulate non-CC-clobbering add/mov insn
patterns? That sounds like it would reduce code quality a lot, unless
great care were taken to make sure all of the save/restore CC flags
insns were optimized away in later passes.

In any case, thanks for the answers so far!

Pip

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj]

Pip Cet writes:

> On Sun, Aug 16, 2020 at 12:50 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>> On Sat, Aug 15, 2020 at 10:18:27AM +0000, Pip Cet wrote:
>> > > > What I'm currently doing is this:
>> > > >
>> > > > (define_split
>> > > >   [(set (match_operand 0 "nonimmediate_operand")
>> > > >     (match_operand 1 "nox_general_operand"))]
>> > > >   "reload_completed && mov_clobbers_cc (insn, operands)"
>> > > >   [(parallel [(set (match_dup 0) (match_dup 1))
>> > > >           (clobber (reg:CC REG_CC))])])
>> > > >
>> > > > which, if I understand correctly, introduces the parallel-clobber
>> > > > expression only when necessary, at the same time as post-reload
>> > > > splitters introduce REG_CC references. Is that correct?
>> > >
>> > > Yes.  And this will work correctly if somehow you ensure that REG_CC
>> > > isn't live anywhere you introduce such a clobber.
>> >
>> > IIUC, the idea is that references to REG_CC, except for clobbers, are
>> > only introduced in the post-reload split pass, so it cannot be live
>> > before our define_split runs. Does that make sense?
>>
>> Yes, it does.  It has some huge restrictions (using the reg in inline
>> assembler can never work reliably, for example, so you'll have to
>> disallow that).
>
> Is there any approach that doesn't suffer from that problem? My
> understanding was that we need to allow reload to insert CC-clobbering
> insns on this (and many other) architectures, and that there are so
> many places where reload might choose to do so (including before and
> after inline asm) that using the register prior to reload just isn't
> possible. I'd be glad to be wrong, though :-)
>
> Is it true that reload chooses which constraint alternative is used
> for each insn? Is that information available somehow to post-reload
> splits? The problem is that the "X" constraint matches whatever's
> there already, and doesn't replace it with the (scratch:CC) expression
> that would work, so I can't rewrite those insns not to clobber the CC
> reg.
>
> For example, here's what I currently have:
>
> (define_expand "mov<mode>"
>   [(parallel [(set (match_operand:MOVMODE 0 "nonimmediate_operand" "")
>            (match_operand:MOVMODE 1 "general_operand" ""))
>           (clobber (reg:CC REG_CC))])]
>   ...)
>
> (define_insn "mov<mode>_insn"
>    [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,r  ,d    ,Qm
>   ,r ,q,r,*r")
>         (match_operand:ALL1 1 "nox_general_operand"   "r,Y00,n Ynn,r
> Y00,Qm,r,q,i"))
>    (clobber (match_scratch:CC 2 "=X,c,X,c,c,X,X,c"))]
>   ...)
>
> That works, but it results in an incorrect CC clobber for, say,
> register-to-register movqi. For that, I'd need something like
>
> (define_split
>   [(parallel [(set (match_operand:ALL1 0 "nonimmediate_operand")
>              (match_operand:ALL1 1 "nox_general_operand"))
>           (clobber (reg:CC REG_CC))])]
>   "reload_completed && REG_P (operands[0]) && REG_P (operands[1])"
>   [(parallel [(set (match_dup 0)
>            (match_dup 1))
>           (clobber (scratch:CC))])])
>
> and so on, for all four constraint alternatives that don't actually
> clobber REG_CC (and also for a fifth which only rarely clobbers
> REG_CC). That's duplicated code, of course.

The (setattr "cc" ...) that is currently present for all
patterns accounts for the constraint alternatives,so  using
get_attr_cc to split to a (clobber) of either cc_reg_rtx or a
gen_rtx_SCRATCH (CCmode) appears to work.

(define_insn_and_split "mov<mode>_insn"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,d    ,Qm   ,r ,q,r,*r")
        (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r Y00,Qm,r,q,i"))]
  "register_operand (operands[0], <MODE>mode)
    || reg_or_0_operand (operands[1], <MODE>mode)"
  "#"
  "&& reload_completed"
  [(parallel [(set (match_dup 0)
                   (match_dup 1))
              (clobber (match_dup 2))])]
  {
    operands[2] = get_cc_reg_clobber_rtx (curr_insn);
  }
  [(set_attr "cc" "ldi,none,clobber,clobber,none,none,clobber")])

(define_insn "*mov<mode>_insn"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,d    ,Qm   ,r ,q,r,*r")
        (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r Y00,Qm,r,q,i"))
   (clobber (match_scratch:CC 2                      "=X    ,X     ,c   ,c ,X,X,c"))]
  "(register_operand (operands[0], <MODE>mode)
    || reg_or_0_operand (operands[1], <MODE>mode))
   && reload_completed"
  {
    return output_movqi (insn, operands, NULL);
  }
  [(set_attr "length" "1,1,5,5,1,1,4")
   (set_attr "adjust_len" "mov8")])

As you mentioned elsewhere, keeping the clobber with "X" helps keep a
single pattern for both CC reg clobbering and non-clobbering insns.


>
> All this is at least somewhat academic: the code produced isn't
> drastically better after my cc conversion, but it's not usually any
> worse, and I'm still looking at assembler examples that are pessimized
> a little to find out how to fix them...
>
>> And earlier passes (like combine) cannot optimise this,
>
> I'm not sure what "this" refers to: my understanding is that the idea
> is to let combine see CC-free code, or code with CC clobbers only,
> which it can then optimize, and only add "real" CC references after
> reload, so many optimizations should just work.
>
>> But it is a pretty straightforward way to move from CC0 to the
>> modern world!
>
> With the limitations of the reload pass being as they are, I don't
> really see a dramatically better alternative. I suppose we could
> explicitly save and restore CC flags around insns emitted when
> reload_in_progress is true, to simulate non-CC-clobbering add/mov insn
> patterns? That sounds like it would reduce code quality a lot, unless
> great care were taken to make sure all of the save/restore CC flags
> insns were optimized away in later passes.
>
> In any case, thanks for the answers so far!
>
> Pip

Re: Clobber REG_CC only for some constraint alternatives?

[Pip Cet]

On Mon, Aug 17, 2020 at 7:31 AM Senthil Kumar Selvaraj
<senthil.thecoder@gmail.com> wrote:
> > (define_split
> >   [(parallel [(set (match_operand:ALL1 0 "nonimmediate_operand")
> >              (match_operand:ALL1 1 "nox_general_operand"))
> >           (clobber (reg:CC REG_CC))])]
> >   "reload_completed && REG_P (operands[0]) && REG_P (operands[1])"
> >   [(parallel [(set (match_dup 0)
> >            (match_dup 1))
> >           (clobber (scratch:CC))])])
> >
> > and so on, for all four constraint alternatives that don't actually
> > clobber REG_CC (and also for a fifth which only rarely clobbers
> > REG_CC). That's duplicated code, of course.
>
> The (setattr "cc" ...) that is currently present for all
> patterns accounts for the constraint alternatives,so  using
> get_attr_cc to split to a (clobber) of either cc_reg_rtx or a
> gen_rtx_SCRATCH (CCmode) appears to work.

Thanks! Using an insn attribute is actually what I ended up doing
(https://github.com/pipcet/gcc/commit/d4509afae9238e0ade4d3e1e97dd8577dae96115)
:-)

It's still confusing, IMHO, that insn attributes (but not the
get_attr_alternative attribute which is mentioned in the
documentation) are available when which_alternative is not.

> (define_insn "*mov<mode>_insn"
>   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,d    ,Qm   ,r ,q,r,*r")
>         (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r Y00,Qm,r,q,i"))
>    (clobber (match_scratch:CC 2                      "=X    ,X     ,c   ,c ,X,X,c"))]

Hmm. Technically, of course, clearing register 0 (a special case of
the first alternative) would clobber the flags, but as it happens, the
rewrite above produces the right clobber expression which is simply
accepted by the "X"... I'm not sure whether anything else is trying to
recognize such insns, but as it stands that define_insn would
recognize the incorrect insn:

[(set (reg:QI 0) (const_int 0))
 (clobber (scratch:CC))]

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj]

Pip Cet writes:

> On Mon, Aug 17, 2020 at 7:31 AM Senthil Kumar Selvaraj
> <senthil.thecoder@gmail.com> wrote:
>> > (define_split
>> >   [(parallel [(set (match_operand:ALL1 0 "nonimmediate_operand")
>> >              (match_operand:ALL1 1 "nox_general_operand"))
>> >           (clobber (reg:CC REG_CC))])]
>> >   "reload_completed && REG_P (operands[0]) && REG_P (operands[1])"
>> >   [(parallel [(set (match_dup 0)
>> >            (match_dup 1))
>> >           (clobber (scratch:CC))])])
>> >
>> > and so on, for all four constraint alternatives that don't actually
>> > clobber REG_CC (and also for a fifth which only rarely clobbers
>> > REG_CC). That's duplicated code, of course.
>>
>> The (setattr "cc" ...) that is currently present for all
>> patterns accounts for the constraint alternatives,so  using
>> get_attr_cc to split to a (clobber) of either cc_reg_rtx or a
>> gen_rtx_SCRATCH (CCmode) appears to work.
>
> Thanks! Using an insn attribute is actually what I ended up doing
> (https://github.com/pipcet/gcc/commit/d4509afae9238e0ade4d3e1e97dd8577dae96115)
> :-)
>
> It's still confusing, IMHO, that insn attributes (but not the
> get_attr_alternative attribute which is mentioned in the
> documentation) are available when which_alternative is not.
>
>> (define_insn "*mov<mode>_insn"
>>   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r    ,d    ,Qm   ,r ,q,r,*r")
>>         (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r Y00,Qm,r,q,i"))
>>    (clobber (match_scratch:CC 2                      "=X    ,X     ,c   ,c ,X,X,c"))]
>
> Hmm. Technically, of course, clearing register 0 (a special case of
> the first alternative) would clobber the flags, but as it happens, the
> rewrite above produces the right clobber expression which is simply
> accepted by the "X"... I'm not sure whether anything else is trying to
> recognize such insns, but as it stands that define_insn would
> recognize the incorrect insn:
>
> [(set (reg:QI 0) (const_int 0))
>  (clobber (scratch:CC))]

get_cc_reg_clobber_rtx also looks at the insn itself (i.e. whatever
the erstwhile NOTICE_UPDATE_CC hook did), so if the cc attribute is LDI,
and operands[1] is const0_rtx, it would return cc_reg_rtx as the clobber reg.

AFAICT, some passes after reload verify if operands match constraints,
and that would work in this case - I'm not sure if the pattern you
mentioned could show up, outside of wrong splitters/peepholes in the md file.

Another approach would be to conservatively use a 'c' constraint and
clobber REG_CC for all reg-reg moves.

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Pip Cet]

On Tue, Aug 18, 2020 at 6:52 AM Senthil Kumar Selvaraj
<senthil.thecoder@gmail.com> wrote:
> > recognize such insns, but as it stands that define_insn would
> > recognize the incorrect insn:
> >
> > [(set (reg:QI 0) (const_int 0))
> >  (clobber (scratch:CC))]
>
> get_cc_reg_clobber_rtx also looks at the insn itself (i.e. whatever
> the erstwhile NOTICE_UPDATE_CC hook did), so if the cc attribute is LDI,
> and operands[1] is const0_rtx, it would return cc_reg_rtx as the clobber reg.
>
> AFAICT, some passes after reload verify if operands match constraints,
> and that would work in this case - I'm not sure if the pattern you
> mentioned could show up, outside of wrong splitters/peepholes in the md file.

I don't think they can, but it's still another case of lying to GCC.

At the very least, output_movqi should assert that it isn't asked to
produce code for this situation.

> Another approach would be to conservatively use a 'c' constraint and
> clobber REG_CC for all reg-reg moves.

I'd prefer splitting the constraint alternatives to have one clear-r0
alternative, an ldi alternative, and a clear -r1_31 alternative.

As for define_subst, is it really worth it? If I'm reading the
documentation correctly, it's not powerful enough to deal with scratch
operands on its own, so we'd probably need three or four variants of
define_subst just to handle those cases.

I'm probably missing something obvious, but what's the reason for
keeping the CC-clobbering post-reload splitter when we already have a
CC-setting one? Are there significant post-reload optimizations that
can deal with a clobber but not an arbitrary assignment to CC? If so,
wouldn't it be better to fix those optimizations?

(There are at least some pre-reload optimizations that don't work with
the CC-clobbering insn patterns. lower_subreg.c's simple_move, for
example, recognizes only two-operand sets. This resulted in pessimized
code, but is easy to fix.)

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj]

Pip Cet writes:

> On Tue, Aug 18, 2020 at 6:52 AM Senthil Kumar Selvaraj
> <senthil.thecoder@gmail.com> wrote:
>> > recognize such insns, but as it stands that define_insn would
>> > recognize the incorrect insn:
>> >
>> > [(set (reg:QI 0) (const_int 0))
>> >  (clobber (scratch:CC))]
>>
>> get_cc_reg_clobber_rtx also looks at the insn itself (i.e. whatever
>> the erstwhile NOTICE_UPDATE_CC hook did), so if the cc attribute is LDI,
>> and operands[1] is const0_rtx, it would return cc_reg_rtx as the clobber reg.
>>
>> AFAICT, some passes after reload verify if operands match constraints,
>> and that would work in this case - I'm not sure if the pattern you
>> mentioned could show up, outside of wrong splitters/peepholes in the md file.
>
> I don't think they can, but it's still another case of lying to GCC.
>
> At the very least, output_movqi should assert that it isn't asked to
> produce code for this situation.

I agree.
>
>> Another approach would be to conservatively use a 'c' constraint and
>> clobber REG_CC for all reg-reg moves.
>
> I'd prefer splitting the constraint alternatives to have one clear-r0
> alternative, an ldi alternative, and a clear -r1_31 alternative.
>
> As for define_subst, is it really worth it? If I'm reading the
> documentation correctly, it's not powerful enough to deal with scratch
> operands on its own, so we'd probably need three or four variants of
> define_subst just to handle those cases.

Is this related to cmpelim not looking at REG_CC clobber if there are
other (clobber scratch..) preceding it?
>
> I'm probably missing something obvious, but what's the reason for
> keeping the CC-clobbering post-reload splitter when we already have a
> CC-setting one? Are there significant post-reload optimizations that
> can deal with a clobber but not an arbitrary assignment to CC? If so,
> wouldn't it be better to fix those optimizations?

The post-reload splitter introduces the clobber. The wiki
suggests that approach if most insns clobber REG_CC, perhaps because of
the missed optimizations you describe below?
>
> (There are at least some pre-reload optimizations that don't work with
> the CC-clobbering insn patterns. lower_subreg.c's simple_move, for
> example, recognizes only two-operand sets. This resulted in pessimized
> code, but is easy to fix.)

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Jeff Law]

On Thu, 2020-08-20 at 21:36 +0530, Senthil Kumar Selvaraj via Gcc wrote:
> Pip Cet writes:
> 
> > On Tue, Aug 18, 2020 at 6:52 AM Senthil Kumar Selvaraj
> > <senthil.thecoder@gmail.com> wrote:
> > > > recognize such insns, but as it stands that define_insn would
> > > > recognize the incorrect insn:
> > > > 
> > > > [(set (reg:QI 0) (const_int 0))
> > > >  (clobber (scratch:CC))]
> > > 
> > > get_cc_reg_clobber_rtx also looks at the insn itself (i.e. whatever
> > > the erstwhile NOTICE_UPDATE_CC hook did), so if the cc attribute is LDI,
> > > and operands[1] is const0_rtx, it would return cc_reg_rtx as the clobber reg.
> > > 
> > > AFAICT, some passes after reload verify if operands match constraints,
> > > and that would work in this case - I'm not sure if the pattern you
> > > mentioned could show up, outside of wrong splitters/peepholes in the md file.
> > 
> > I don't think they can, but it's still another case of lying to GCC.
> > 
> > At the very least, output_movqi should assert that it isn't asked to
> > produce code for this situation.
> 
> I agree.
> > > Another approach would be to conservatively use a 'c' constraint and
> > > clobber REG_CC for all reg-reg moves.
> > 
> > I'd prefer splitting the constraint alternatives to have one clear-r0
> > alternative, an ldi alternative, and a clear -r1_31 alternative.
> > 
> > As for define_subst, is it really worth it? If I'm reading the
> > documentation correctly, it's not powerful enough to deal with scratch
> > operands on its own, so we'd probably need three or four variants of
> > define_subst just to handle those cases.
> 
> Is this related to cmpelim not looking at REG_CC clobber if there are
> other (clobber scratch..) preceding it?
> > I'm probably missing something obvious, but what's the reason for
> > keeping the CC-clobbering post-reload splitter when we already have a
> > CC-setting one? Are there significant post-reload optimizations that
> > can deal with a clobber but not an arbitrary assignment to CC? If so,
> > wouldn't it be better to fix those optimizations?
> 
> The post-reload splitter introduces the clobber. The wiki
> suggests that approach if most insns clobber REG_CC, perhaps because of
> the missed optimizations you describe below?
If most patterns set/clobber the flags, then yes, it's slightly better to only
expose them after reload.   Various passes that directly grub through RTL rather
than using helpers like single_set will optimize things better.

That's the approach I've taken with the H8 conversion, which is basically working
at this point and when I have the time I walk through various minor codegen
inefficiences.

Jeff

Re: Clobber REG_CC only for some constraint alternatives?

[Hans-Peter Nilsson]

On Mon, 24 Aug 2020, Jeff Law via Gcc wrote:
> On Thu, 2020-08-20 at 21:36 +0530, Senthil Kumar Selvaraj via Gcc wrote:
> > The post-reload splitter introduces the clobber. The wiki
> > suggests that approach if most insns clobber REG_CC, perhaps because of
> > the missed optimizations you describe below?
> If most patterns set/clobber the flags, then yes, it's slightly better to only
> expose them after reload.   Various passes that directly grub through RTL rather
> than using helpers like single_set will optimize things better.

The "slightly" being omissions like the one fixed in combine.c
last month. ;-)  There's one in reload too ("Discard obvious
no-ops, even without -O"), but it seems to be acting the other
way(!)

If, for a machine where most insns clobber REG_CC, you instead
emit with the clobber initially, you can have a tidier port: one
define_subst:ed pattern instead of...  How many is it with the
only-expose-after-reload method (counting define_insn,
define_split, possibly a define_expand too that wasn't there for
cc0)?  Two or three, per (original) insn with cc0?

brgds, H-P

Re: Clobber REG_CC only for some constraint alternatives?

[Jeff Law]

On Tue, 2020-08-25 at 23:58 -0400, Hans-Peter Nilsson wrote:
> On Mon, 24 Aug 2020, Jeff Law via Gcc wrote:
> > On Thu, 2020-08-20 at 21:36 +0530, Senthil Kumar Selvaraj via Gcc wrote:
> > > The post-reload splitter introduces the clobber. The wiki
> > > suggests that approach if most insns clobber REG_CC, perhaps because of
> > > the missed optimizations you describe below?
> > If most patterns set/clobber the flags, then yes, it's slightly better to only
> > expose them after reload.   Various passes that directly grub through RTL rather
> > than using helpers like single_set will optimize things better.
> 
> The "slightly" being omissions like the one fixed in combine.c
> last month. ;-)  There's one in reload too ("Discard obvious
> no-ops, even without -O"), but it seems to be acting the other
> way(!)
Yea, this is the kind of thing I'm thinking of when I say "slightly".

> 
> If, for a machine where most insns clobber REG_CC, you instead
> emit with the clobber initially, you can have a tidier port: one
> define_subst:ed pattern instead of...  How many is it with the
> only-expose-after-reload method (counting define_insn,
> define_split, possibly a define_expand too that wasn't there for
> cc0)?  Two or three, per (original) insn with cc0?
It depends :-)  On the H8 I've tried to focus efforts on creating the additional
patterns only when they're actually helpful.  So it'd normally be 2X or 3X the
number of patterns, but it's slightly less on the H8.

I pondered a define_subst approach, but with my lack of experience with
define_subst and the head start from my son on the mechanical changes it seemed
easier to just continue without define_subst.

jeff

Re: Clobber REG_CC only for some constraint alternatives?

[Hans-Peter Nilsson]

On Wed, 26 Aug 2020, Jeff Law wrote:

> On Tue, 2020-08-25 at 23:58 -0400, Hans-Peter Nilsson wrote:
> > On Mon, 24 Aug 2020, Jeff Law via Gcc wrote:
> > > On Thu, 2020-08-20 at 21:36 +0530, Senthil Kumar Selvaraj via Gcc wrote:
> > > > The post-reload splitter introduces the clobber. The wiki
> > > > suggests that approach if most insns clobber REG_CC, perhaps because of
> > > > the missed optimizations you describe below?
> > > If most patterns set/clobber the flags, then yes, it's slightly better to only
> > > expose them after reload.   Various passes that directly grub through RTL rather
> > > than using helpers like single_set will optimize things better.
> >
> > The "slightly" being omissions like the one fixed in combine.c
> > last month. ;-)  There's one in reload too ("Discard obvious
> > no-ops, even without -O"), but it seems to be acting the other
> > way(!)
> Yea, this is the kind of thing I'm thinking of when I say "slightly".

JFTR, I'd like to stress that point: contrary to my initial
fears, my experience with the CRIS de-cc0ration is that the
straight-set insn assumptions in gcc are few and far-between
enough to be ignored as a factor regarding missed optimizations,
when picking a strategy.

brgds, H-P

Re: Clobber REG_CC only for some constraint alternatives?

[Pip Cet]

On Mon, Aug 24, 2020 at 6:18 PM Jeff Law <law@redhat.com> wrote:
> > The post-reload splitter introduces the clobber. The wiki
> > suggests that approach if most insns clobber REG_CC, perhaps because of
> > the missed optimizations you describe below?
> If most patterns set/clobber the flags, then yes, it's slightly better to only
> expose them after reload.   Various passes that directly grub through RTL rather
> than using helpers like single_set will optimize things better.

I think I made it to the next pitfall :-)

The cmpelim pass tries to recognize cc-setting variants of insns.
Whether or not there is one (i.e. whether or not the insn should be
recognized) depends on the "cc" attribute, which depends on which
alternative is used. So I did the obvious thing, and put a condition
in the define_insn which depends on get_cc_attr (insn). But
get_cc_attr() tries to recognize the insn, so we recurse indefinitely
and die with a segfault.

Things appear to work with a somewhat subtle hack: we recognize that a
false positive from the inner recognition isn't harmful, because the
outer condition will still catch invalid cases.

  static int recurse = 0;
  if (recurse)
    return gen_rtx_REG (CCmode, REG_CC); // causes the insn to be recognized
  recurse++;
  int old_insn_code = INSN_CODE (insn);
  enum attr_cc cc = get_attr_cc (insn);
  INSN_CODE (insn) = old_insn_code;
  recurse--;

But surely that's not the right way?

Note that whether there is a CC-setting variant depends not just on
the "cc" attr, but also on the precise operands for some values of the
"cc" attr, which requires hairy C code to figure out.

Is it possible to avoid this situation by avoiding constraint
alternatives, and defining insns separately for each of them?

Re: Clobber REG_CC only for some constraint alternatives?

[Jeff Law]

On Wed, 2020-08-26 at 11:18 +0000, Pip Cet wrote:
> On Mon, Aug 24, 2020 at 6:18 PM Jeff Law <law@redhat.com> wrote:
> > > The post-reload splitter introduces the clobber. The wiki
> > > suggests that approach if most insns clobber REG_CC, perhaps because of
> > > the missed optimizations you describe below?
> > If most patterns set/clobber the flags, then yes, it's slightly better to only
> > expose them after reload.   Various passes that directly grub through RTL rather
> > than using helpers like single_set will optimize things better.
> 
> I think I made it to the next pitfall :-)
> 
> The cmpelim pass tries to recognize cc-setting variants of insns.
> Whether or not there is one (i.e. whether or not the insn should be
> recognized) depends on the "cc" attribute, which depends on which
> alternative is used. So I did the obvious thing, and put a condition
> in the define_insn which depends on get_cc_attr (insn). But
> get_cc_attr() tries to recognize the insn, so we recurse indefinitely
> and die with a segfault.
> 
> Things appear to work with a somewhat subtle hack: we recognize that a
> false positive from the inner recognition isn't harmful, because the
> outer condition will still catch invalid cases.
> 
>   static int recurse = 0;
>   if (recurse)
>     return gen_rtx_REG (CCmode, REG_CC); // causes the insn to be recognized
>   recurse++;
>   int old_insn_code = INSN_CODE (insn);
>   enum attr_cc cc = get_attr_cc (insn);
>   INSN_CODE (insn) = old_insn_code;
>   recurse--;
> 
> But surely that's not the right way?
> 
> Note that whether there is a CC-setting variant depends not just on
> the "cc" attr, but also on the precise operands for some values of the
> "cc" attr, which requires hairy C code to figure out.
> 
> Is it possible to avoid this situation by avoiding constraint
> alternatives, and defining insns separately for each of them?
You can split most things up like you've suggested.  The exception would be the
movXX patterns.

Jeff

Re: Clobber REG_CC only for some constraint alternatives?

[Jeff Law]

On Thu, 2020-08-27 at 16:31 +0530, Senthil Kumar Selvaraj wrote:
> Pip Cet writes:
> 
> > On Mon, Aug 24, 2020 at 6:18 PM Jeff Law <law@redhat.com> wrote:
> > > > The post-reload splitter introduces the clobber. The wiki
> > > > suggests that approach if most insns clobber REG_CC, perhaps because of
> > > > the missed optimizations you describe below?
> > > If most patterns set/clobber the flags, then yes, it's slightly better to only
> > > expose them after reload.   Various passes that directly grub through RTL rather
> > > than using helpers like single_set will optimize things better.
> > 
> > I think I made it to the next pitfall :-)
> > 
> > The cmpelim pass tries to recognize cc-setting variants of insns.
> > Whether or not there is one (i.e. whether or not the insn should be
> > recognized) depends on the "cc" attribute, which depends on which
> > alternative is used. So I did the obvious thing, and put a condition
> > in the define_insn which depends on get_cc_attr (insn). But
> > get_cc_attr() tries to recognize the insn, so we recurse indefinitely
> > and die with a segfault.
> > 
> > Things appear to work with a somewhat subtle hack: we recognize that a
> > false positive from the inner recognition isn't harmful, because the
> > outer condition will still catch invalid cases.
> > 
> >   static int recurse = 0;
> >   if (recurse)
> >     return gen_rtx_REG (CCmode, REG_CC); // causes the insn to be recognized
> >   recurse++;
> >   int old_insn_code = INSN_CODE (insn);
> >   enum attr_cc cc = get_attr_cc (insn);
> >   INSN_CODE (insn) = old_insn_code;
> >   recurse--;
> > 
> > But surely that's not the right way?
> > 
> > Note that whether there is a CC-setting variant depends not just on
> > the "cc" attr, but also on the precise operands for some values of the
> > "cc" attr, which requires hairy C code to figure out.
> 
> This is only for ldi and plus cc attribute values, right? Can we
> conservatively have only the clobber, and see how it impacts generated code?
You should always be able to claim its clobbered, to get things running, then
evaluate if it's worth trying to model that case more accurately to get better
compare elimination.

jeff

Re: Clobber REG_CC only for some constraint alternatives?

[Hans-Peter Nilsson]

On Wed, 26 Aug 2020, Pip Cet via Gcc wrote:

> Note that whether there is a CC-setting variant depends not just on
> the "cc" attr, but also on the precise operands for some values of the
> "cc" attr, which requires hairy C code to figure out.
>
> Is it possible to avoid this situation by avoiding constraint
> alternatives, and defining insns separately for each of them?

Well, rather than "avoid this situation", fix it.

Besides defaulting to clobber and separate splitters like Jeff
wrote, the best action here is to make the "cc" attribute
accurate when non-clobber.  That is, make the "hairy C code"
part of the alternative-matching-expression (typically the
constraint combination).

brgds, H-P

Re: Clobber REG_CC only for some constraint alternatives?

[Segher Boessenkool]

Hi!

On Sun, Aug 16, 2020 at 06:28:44PM +0000, Pip Cet wrote:
> > > IIUC, the idea is that references to REG_CC, except for clobbers, are
> > > only introduced in the post-reload split pass, so it cannot be live
> > > before our define_split runs. Does that make sense?
> >
> > Yes, it does.  It has some huge restrictions (using the reg in inline
> > assembler can never work reliably, for example, so you'll have to
> > disallow that).
> 
> Is there any approach that doesn't suffer from that problem? My
> understanding was that we need to allow reload to insert CC-clobbering
> insns on this (and many other) architectures, and that there are so
> many places where reload might choose to do so (including before and
> after inline asm) that using the register prior to reload just isn't
> possible. I'd be glad to be wrong, though :-)

You can insert the clobber at expand time already.  That probably works
worse if most of your insns clobber the flags, but better if many do
not.

> Is it true that reload chooses which constraint alternative is used
> for each insn?

Register allocation does, yes (reload (or LRA) is the last step of that,
it has the last say).

> Is that information available somehow to post-reload
> splits?

In the "which_alternative" variable (not an attribute, an actual
variable; so use it from a C block or C expression).  There also is the
"alternative" attribute (which uses "which_alternative" under the hood),
which is handy for defining *other* attributes.

> The problem is that the "X" constraint matches whatever's
> there already, and doesn't replace it with the (scratch:CC) expression
> that would work, so I can't rewrite those insns not to clobber the CC
> reg.
> 
> For example, here's what I currently have:
> 
> (define_expand "mov<mode>"
>   [(parallel [(set (match_operand:MOVMODE 0 "nonimmediate_operand" "")
>            (match_operand:MOVMODE 1 "general_operand" ""))
>           (clobber (reg:CC REG_CC))])]
>   ...)

Yeah, if you use a hard register at expand time already, it won't be
changed by register allocation.

> (define_insn "mov<mode>_insn"
>    [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,r  ,d    ,Qm
>   ,r ,q,r,*r")
>         (match_operand:ALL1 1 "nox_general_operand"   "r,Y00,n Ynn,r
> Y00,Qm,r,q,i"))
>    (clobber (match_scratch:CC 2 "=X,c,X,c,c,X,X,c"))]
>   ...)
> 
> That works, but it results in an incorrect CC clobber for, say,
> register-to-register movqi. For that, I'd need something like
> 
> (define_split
>   [(parallel [(set (match_operand:ALL1 0 "nonimmediate_operand")
>              (match_operand:ALL1 1 "nox_general_operand"))
>           (clobber (reg:CC REG_CC))])]
>   "reload_completed && REG_P (operands[0]) && REG_P (operands[1])"
>   [(parallel [(set (match_dup 0)
>            (match_dup 1))
>           (clobber (scratch:CC))])])
> 
> and so on, for all four constraint alternatives that don't actually
> clobber REG_CC (and also for a fifth which only rarely clobbers
> REG_CC). That's duplicated code, of course.

You can make it a define_insn_and_split where the condition can look
at the actual operands, or can use which_alternative.

> All this is at least somewhat academic: the code produced isn't
> drastically better after my cc conversion, but it's not usually any
> worse, and I'm still looking at assembler examples that are pessimized
> a little to find out how to fix them...

Great :-)

> > And earlier passes (like combine) cannot optimise this,
> 
> I'm not sure what "this" refers to: my understanding is that the idea
> is to let combine see CC-free code, or code with CC clobbers only,
> which it can then optimize, and only add "real" CC references after
> reload, so many optimizations should just work.

"This" was meant to refer to "anything to do with the CC reg".  If it
doesn't yet exist until after reload, of course no pass before it can do
anything with it :-)

> > But it is a pretty straightforward way to move from CC0 to the
> > modern world!
> 
> With the limitations of the reload pass being as they are, I don't
> really see a dramatically better alternative. I suppose we could
> explicitly save and restore CC flags around insns emitted when
> reload_in_progress is true, to simulate non-CC-clobbering add/mov insn
> patterns? That sounds like it would reduce code quality a lot, unless
> great care were taken to make sure all of the save/restore CC flags
> insns were optimized away in later passes.

Do you have move insns that do not clobber the CC reg?  If not, good
luck :-)

> In any case, thanks for the answers so far!

My pleasure.


Segher

Re: Clobber REG_CC only for some constraint alternatives?

[Hans-Peter Nilsson]

On Sun, 16 Aug 2020, Pip Cet via Gcc wrote:

> For example, here's what I currently have:
>
> (define_expand "mov<mode>"
>   [(parallel [(set (match_operand:MOVMODE 0 "nonimmediate_operand" "")
>            (match_operand:MOVMODE 1 "general_operand" ""))
>           (clobber (reg:CC REG_CC))])]
>   ...)
>
> (define_insn "mov<mode>_insn"
>    [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,r  ,d    ,Qm
>   ,r ,q,r,*r")
>         (match_operand:ALL1 1 "nox_general_operand"   "r,Y00,n Ynn,r
> Y00,Qm,r,q,i"))
>    (clobber (match_scratch:CC 2 "=X,c,X,c,c,X,X,c"))]
>   ...)
>
> That works, but it results in an incorrect CC clobber for, say,
> register-to-register movqi. For that, I'd need something like

Another way to re-use an existing "cc" attribute from cc0 times
is like I did in a82c9fb3f70 (supporting machinery and movsi
usage), minimizing code edits and duplication when using it for
CC-setting insns.  For an example of that, see the subsequent
commit a33649e6664.  (Just decorate the insn name with the
appropriate define_subst ornament!)

I'll be quiet now, but good luck.

brgds, H-P

Re: Clobber REG_CC only for some constraint alternatives?

[Hans-Peter Nilsson]

On Fri, 14 Aug 2020, Senthil Kumar Selvaraj via Gcc wrote:
> As you can deduce from the (set_attr "cc" ..), only constraint
> alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.

Yes, I recognize that.

> My first version of the port adds a post-reload splitter that adds a
> (clobber (reg:CC REG_CC)) unconditionally, and it appears to work.

Ouch, temporarily lying to gcc here.

> If I
> do want to add the clobber conditionally, would something like the below
> be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
> or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner way?

I suggest having a look at what I did for the CRIS port.
Check the git history.

In short:
- Add the clobber initially, to *all* patterns.
- Add postreload splitters for the special combinations that
don't clobber CC (ones without clobbering CC).
- Use the old "cc" attribute to control and generate
clobber/useful CC-setting alternatives (for various new CC_
modes) with use of define_subst.

No regressions in CC quality (with one pending patch).

brgds, H-P

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj]

Hans-Peter Nilsson writes:

> On Fri, 14 Aug 2020, Senthil Kumar Selvaraj via Gcc wrote:
>> As you can deduce from the (set_attr "cc" ..), only constraint
>> alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.
>
> Yes, I recognize that.
>
>> My first version of the port adds a post-reload splitter that adds a
>> (clobber (reg:CC REG_CC)) unconditionally, and it appears to work.
>
> Ouch, temporarily lying to gcc here.
>
>> If I
>> do want to add the clobber conditionally, would something like the below
>> be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
>> or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner way?
>
> I suggest having a look at what I did for the CRIS port.
> Check the git history.
>
> In short:
> - Add the clobber initially, to *all* patterns.
> - Add postreload splitters for the special combinations that
> don't clobber CC (ones without clobbering CC).
> - Use the old "cc" attribute to control and generate
> clobber/useful CC-setting alternatives (for various new CC_
> modes) with use of define_subst.

This sounds like a great plan - the idea of always generating insn
variants for however many CCmodes are available, and then using
cc_enabled  to disable them selectively (based on the attribute value
corresponding to the alternative chosen) looks really neat. I did not
understand the purpose of subst_attr for "ccnz" "ccnzvc" and "cccc"
though - wouldn't a (set_attr "cc_enabled", "...") do the same thing?

Also, I already have a version that hides REG_CC until reload (based on
the suggestion in the wiki page), but I guess this approach will work
equally well with that too?

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Hans-Peter Nilsson]

On Wed, 19 Aug 2020, Senthil Kumar Selvaraj wrote:
>
> Hans-Peter Nilsson writes:
>
> > On Fri, 14 Aug 2020, Senthil Kumar Selvaraj via Gcc wrote:
> >> As you can deduce from the (set_attr "cc" ..), only constraint
> >> alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.
> >
> > Yes, I recognize that.
> >
> >> My first version of the port adds a post-reload splitter that adds a
> >> (clobber (reg:CC REG_CC)) unconditionally, and it appears to work.
> >
> > Ouch, temporarily lying to gcc here.
> >
> >> If I
> >> do want to add the clobber conditionally, would something like the below
> >> be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
> >> or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner way?
> >
> > I suggest having a look at what I did for the CRIS port.
> > Check the git history.
> >
> > In short:
> > - Add the clobber initially, to *all* patterns.
> > - Add postreload splitters for the special combinations that
> > don't clobber CC (ones without clobbering CC).
> > - Use the old "cc" attribute to control and generate
> > clobber/useful CC-setting alternatives (for various new CC_
> > modes) with use of define_subst.
>
> This sounds like a great plan - the idea of always generating insn
> variants for however many CCmodes are available, and then using
> cc_enabled  to disable them selectively (based on the attribute value
> corresponding to the alternative chosen) looks really neat. I did not
> understand the purpose of subst_attr for "ccnz" "ccnzvc" and "cccc"
> though - wouldn't a (set_attr "cc_enabled", "...") do the same thing?

You mean adding a whole new line with separate per-alternative
settings instead of just a suffix to the name, per instruction,
to get it compare-elim-optimized?  (Ok, plus a tweak to
SELECT_CC_MODE; see a33649e).  That's not simpler to me, and
error-prone without benefits.  Note that the "cc" attribute was
already there from cc0 times, just as for AVR!

But beware, the cc attributes *might* need tweaking, and also
note that disabling an alternative for an insn may make gcc pick
a subsequent one, which may be invalid when matching operands
for the disabled alternative.

But that's the simplistic and obvious reply, perhaps I'm
misinterpreting and you mean something else?  For the purpose of
the different CCmodes, see cris-modes.def, in particular the
second paragraph, which I'm not repeating here.

> Also, I already have a version that hides REG_CC until reload (based on
> the suggestion in the wiki page), but I guess this approach will work
> equally well with that too?

Ow, I see it does!  I don't know, that's unchartered territory
to me, but it does seem like visium was written that way.  It
seems a bit wordy though; the define_split isn't necessary if
you have the clobber from the start and use define_subst for the
other alternatives.  You still need splitters for special-cases
of insns where condition codes aren't affected though.  If these
"special cases" approach being the norm, then I don't know.

IMHO, introducing REG_CC clobbers only *after* reload seems like
it could backfire.  Doing things behind gcc's back is what we're
eliminating, not re-introducing, to be dogmatic.

That wiki page mentions using define_subst to avoid all
(near-)duplicates, but not in its examples.

brgds, H-P

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj]

Hans-Peter Nilsson writes:

> On Wed, 19 Aug 2020, Senthil Kumar Selvaraj wrote:
>>
>> Hans-Peter Nilsson writes:
>>
>> > On Fri, 14 Aug 2020, Senthil Kumar Selvaraj via Gcc wrote:
>> >> As you can deduce from the (set_attr "cc" ..), only constraint
>> >> alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.
>> >
>> > Yes, I recognize that.
>> >
>> >> My first version of the port adds a post-reload splitter that adds a
>> >> (clobber (reg:CC REG_CC)) unconditionally, and it appears to work.
>> >
>> > Ouch, temporarily lying to gcc here.
>> >
>> >> If I
>> >> do want to add the clobber conditionally, would something like the below
>> >> be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
>> >> or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner way?
>> >
>> > I suggest having a look at what I did for the CRIS port.
>> > Check the git history.
>> >
>> > In short:
>> > - Add the clobber initially, to *all* patterns.
>> > - Add postreload splitters for the special combinations that
>> > don't clobber CC (ones without clobbering CC).
>> > - Use the old "cc" attribute to control and generate
>> > clobber/useful CC-setting alternatives (for various new CC_
>> > modes) with use of define_subst.
>>
>> This sounds like a great plan - the idea of always generating insn
>> variants for however many CCmodes are available, and then using
>> cc_enabled  to disable them selectively (based on the attribute value
>> corresponding to the alternative chosen) looks really neat. I did not
>> understand the purpose of subst_attr for "ccnz" "ccnzvc" and "cccc"
>> though - wouldn't a (set_attr "cc_enabled", "...") do the same thing?
>
> You mean adding a whole new line with separate per-alternative
> settings instead of just a suffix to the name, per instruction,
> to get it compare-elim-optimized?  (Ok, plus a tweak to
> SELECT_CC_MODE; see a33649e).  That's not simpler to me, and
> error-prone without benefits.  Note that the "cc" attribute was
> already there from cc0 times, just as for AVR!
>
> But beware, the cc attributes *might* need tweaking, and also
> note that disabling an alternative for an insn may make gcc pick
> a subsequent one, which may be invalid when matching operands
> for the disabled alternative.
>
> But that's the simplistic and obvious reply, perhaps I'm
> misinterpreting and you mean something else?  For the purpose of
> the different CCmodes, see cris-modes.def, in particular the
> second paragraph, which I'm not repeating here.

My question was a lot more basic. From the internals documentation,
I understand that, with define_subst_attr for setcc, setnz and setnzc,
and the corresponding define_substs,

(define_insn "*movsi_internal<setcc><setnz><setnzvc> ...)

results in a

(define_insn "*movsi_internal ...)

and in addition to that,

(define_insn "*movsi_internal_setcc ...) // output template of setcc_subst
(define_insn "*movsi_internal_setnz ...) // output template of setnz_subst
(define_insn "*movsi_internal_setnzvc ...) // output template of setnzvc_subst

I understood why this was done.

What I didn't understand was the (set-attr "cc<cccc><ccnz><ccnzvc>")
part - as far I can tell, this results in (set_attr "cc_enabled" ...) in
all of the three substituted patterns, so I wondered why not just have
(set_attr "cc_enabled" ...) in the original define_insn instead.

I now realize that with (set-attr "cc<cccc><ccnz><ccnzvc>"), the original
unsubstituted pattern will have only a (set_attr "cc" ...) and would
therefore not match the attr check for "enabled" - correctly so, as the
original insn pattern clobbers CRIS_CC0_REGNUM. Did I get that right?

>
>> Also, I already have a version that hides REG_CC until reload (based on
>> the suggestion in the wiki page), but I guess this approach will work
>> equally well with that too?
>
> Ow, I see it does!  I don't know, that's unchartered territory
> to me, but it does seem like visium was written that way.  It
> seems a bit wordy though; the define_split isn't necessary if
> you have the clobber from the start and use define_subst for the
> other alternatives.  You still need splitters for special-cases
> of insns where condition codes aren't affected though.  If these
> "special cases" approach being the norm, then I don't know.

Ok, I'll try the cris approach too, and see if it makes any difference.
>
> IMHO, introducing REG_CC clobbers only *after* reload seems like
> it could backfire.  Doing things behind gcc's back is what we're
> eliminating, not re-introducing, to be dogmatic.
>
> That wiki page mentions using define_subst to avoid all
> (near-)duplicates, but not in its examples.

Yes, I saw that too.

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Andrew Stubbs]

On 20/08/2020 06:40, Senthil Kumar Selvaraj via Gcc wrote:
> What I didn't understand was the (set-attr "cc<cccc><ccnz><ccnzvc>")
> part - as far I can tell, this results in (set_attr "cc_enabled" ...) in
> all of the three substituted patterns, so I wondered why not just have
> (set_attr "cc_enabled" ...) in the original define_insn instead.
> 
> I now realize that with (set-attr "cc<cccc><ccnz><ccnzvc>"), the original
> unsubstituted pattern will have only a (set_attr "cc" ...) and would
> therefore not match the attr check for "enabled" - correctly so, as the
> original insn pattern clobbers CRIS_CC0_REGNUM. Did I get that right?

The best (only?) way to understand define_subst is to read the expanded 
machine description. This is not written anywhere, by default, but 
there's a way to get it.

   cd <build>/gcc
   make mddump
   less tmp-mddump.md

Not only are all the define_subst expanded, but so are all the other 
iterators.

HTH

Andrew

Re: Clobber REG_CC only for some constraint alternatives?

[H.J. Lu]

On Thu, Aug 20, 2020 at 1:53 AM Andrew Stubbs <ams@codesourcery.com> wrote:
>
> On 20/08/2020 06:40, Senthil Kumar Selvaraj via Gcc wrote:
> > What I didn't understand was the (set-attr "cc<cccc><ccnz><ccnzvc>")
> > part - as far I can tell, this results in (set_attr "cc_enabled" ...) in
> > all of the three substituted patterns, so I wondered why not just have
> > (set_attr "cc_enabled" ...) in the original define_insn instead.
> >
> > I now realize that with (set-attr "cc<cccc><ccnz><ccnzvc>"), the original
> > unsubstituted pattern will have only a (set_attr "cc" ...) and would
> > therefore not match the attr check for "enabled" - correctly so, as the
> > original insn pattern clobbers CRIS_CC0_REGNUM. Did I get that right?
>
> The best (only?) way to understand define_subst is to read the expanded
> machine description. This is not written anywhere, by default, but
> there's a way to get it.
>
>    cd <build>/gcc
>    make mddump
>    less tmp-mddump.md
>
> Not only are all the define_subst expanded, but so are all the other
> iterators.

Shouldn't it be documented?

-- 
H.J.

Re: Clobber REG_CC only for some constraint alternatives?

[Hans-Peter Nilsson]

On Thu, 20 Aug 2020, Senthil Kumar Selvaraj wrote:

> What I didn't understand was the (set-attr "cc<cccc><ccnz><ccnzvc>")
> part - as far I can tell, this results in (set_attr "cc_enabled" ...) in
> all of the three substituted patterns, so I wondered why not just have
> (set_attr "cc_enabled" ...) in the original define_insn instead.
>
> I now realize that with (set-attr "cc<cccc><ccnz><ccnzvc>"), the original
> unsubstituted pattern will have only a (set_attr "cc" ...) and would
> therefore not match the attr check for "enabled" - correctly so, as the
> original insn pattern clobbers CRIS_CC0_REGNUM. Did I get that right?

Aha: yes, correct.  Attribute "cc" is now "unused" in the
original (un-subst:ed) insn.  Commit a82c9fb3.

brgds, H-P

Re: Clobber REG_CC only for some constraint alternatives?

[Jeff Law]

On Fri, 2020-08-14 at 16:46 +0530, Senthil Kumar Selvaraj wrote:
> Hi,
> 
>   I'm working on porting AVR to MODE_CC, and there are quite a few
>   patterns that clobber the condition code reg only for certain
>   constraint alternatives. For e.g.,
Yea.  H8 has the same issue.  It's worth noting that showing a clobber for the CC
register even when the bits aren't clobbered is safe.

So I started with that on the H8, even though it did result in some code
efficiency regressions.  Then I started pulling apart those patterns when testing
showed it was worth the effort.

Jeff