RE: [PATCH][gensupport] Add optional attributes field to define_cond_exec

["Kyrylo Tkachov" <kyrylo dot tkachov at arm dot com>]

Hi Michael,

> Hi Kyrylo, Richard,
> 
> > What would be the function of (set_attr "ds_predicable" "yes") ?
> > Doesn't the use of <ds_predicable_enabled> already trigger the
> substitution?
> To use define subst one doesn't need to write (set_attr
> "ds_predicable" "yes") - it's triggered by mentioning any of connected
> subst-attributes in the pattern.
> 
> > ... But I'd like to keep using the
> > "predicable" attribute
> > the way it's used now to mark patterns for cond_exec'ednes.
> If you decide to move to define_subst from cond_exec, then I'd suggest
> to avoid using 'predicable' attribute - it could involve cond_exec
> after or before define_subst and that's definitely not what you might
> want to get.

I'm reluctant to replace define_cond_exec with define_subst for a couple of
reasons:

- What about define_insn_and_split? Currently, we can define "predicable"
for a define_insn_and_split,
but the define_subst documentation says it can only be applied to
define_insn and define_expand.

- What about predication on a per-alternative basis (set_attr "predicable"
"yes,no,yes")?
If the presence of a subst_attr in a pattern triggers substitution (and
hence predication),
how do we specify that a particular alternative cannot be predicable? the
define_cond_exec
machinery does some implicit tricks with ce_enabled and noce_enabled that
allows to do that
(http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00094.html).
define_subst doesn't seem like a good substitute (no pun intended ;) ) at
this point.

> 
> If you describe in more details, which patterns you're trying to get
> from which, I'll try to help with define_subst.

An example of what I'm trying to achieve is here:
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01139.html

So, define_cond_exec is used in the arm backend as follows:
(define_cond_exec
  [(match_operator 0 "arm_comparison_operator"
    [(match_operand 1 "cc_register" "")
     (const_int 0)])]
  "TARGET_32BIT"
  ""
[(set_attr "predicated" "yes")]
)

If I were to replace it with a define_subst, as per Richards' suggestion, it
would look like this?

(define_subst "ds_predicable"
  [(match_operand 0)]
  "TARGET_32BIT"
  [(cond_exec (match_operator 1 "arm_comparison_operator"
                [(match_operand 2 "cc_register" "")
                 (const_int 0)])
              (match_dup 0))])

(define_subst_attr "ds_predicable_enabled" "ds_predicable" "no" "yes")

Then, a pattern like:

(define_insn "arm_load_exclusive<mode>"
  [(set (match_operand:SI 0 "s_register_operand" "=r")
        (zero_extend:SI
	  (unspec_volatile:NARROW
	    [(match_operand:NARROW 1 "mem_noofs_operand" "Ua")]
	    VUNSPEC_LL)))]
  "TARGET_HAVE_LDREXBH"
  "ldrex<sync_sfx>%?\t%0, %C1"
  [(set_attr "predicable" "yes")])

would be rewritten like this:

(define_insn "arm_load_exclusive<mode>"
  [(set (match_operand:SI 0 "s_register_operand" "=r")
        (zero_extend:SI
	  (unspec_volatile:NARROW
	    [(match_operand:NARROW 1 "mem_noofs_operand" "Ua")]
	    VUNSPEC_LL)))]
  "TARGET_HAVE_LDREXBH"
  "ldrex<sync_sfx>%?\t%0, %C1"
  [(set_attr "predicated" "<ds_predicable_enabled>")])

The substitution is triggered implicitly when <ds_predicable_enabled> is
encountered.
The "predicable" attribute is gone
But what if there was a second alternative in this define_insn that was
originally non-predicable?
(i.e. (set_attr "predicable" "yes, no")). How would we ensure that the
cond_exec version of that is
not produced?

It seems to me that, as things stand currently, adding the capability to add
set_attr to define_cond_exec
(what my patch does) is the cleaner solution from a backend perspective,
requiring fewer rewrites/workarounds
for dealing with cond_exec's.


Thanks,
Kyrill
> 
> Thanks, Michael
> 
> On 23 May 2013 12:56, Kyrylo Tkachov <kyrylo.tkachov@arm.com> wrote:
> > Hi Richard,
> >
> >> No, define_subst works across patterns, keyed by attributes.
> Exactly
> >> like
> >> cond_exec, really.
> >>
> >> But what you ought to be able to do right now is
> >>
> >> (define_subst "ds_predicable"
> >>   [(match_operand 0)]
> >>   ""
> >>   [(cond_exec (blah) (match_dup 0))])
> >>
> >> (define_subst_attr "ds_predicable_enabled" "ds_predicable" "no"
> "yes"0
> >>
> >> (define_insn "blah"
> >>   [(blah)]
> >>   ""
> >>   "@
> >>    blah
> >>    blah"
> >>   [(set_attr "ds_predicable" "yes")
> >>    (set_attr "ds_predicated" "<ds_predicable_enabled>")])
> >
> > What would be the function of (set_attr "ds_predicable" "yes") ?
> > Doesn't the use of <ds_predicable_enabled> already trigger the
> substitution?
> >
> >>
> >> At which point you can define "enabled" in terms of ds_predicated
> plus
> >> whatever.
> >>
> >> With a small bit of work we ought to be able to move that
> ds_predicated
> >> attribute to the define_subst itself, so that you don't have to
> >> replicate that
> >> set_attr line N times.
> >
> > That would be nice. So we would have to use define_subst instead of
> > define_cond_exec
> > to generate the cond_exec patterns. But I'd like to keep using the
> > "predicable" attribute
> > the way it's used now to mark patterns for cond_exec'ednes.
> >
> > So you'd recommend changing the define_subst machinery to handle that
> > ds_predicated attribute?
> >
> >
> >   I think that's more or less what you were
> >> suggesting
> >> with your cond_exec extension, yes?
> >
> > Pretty much, yes. Thanks for the explanation.