[PATCH][gensupport] Add optional attributes field to define_cond_exec
Kyrylo Tkachov
kyrylo.tkachov@arm.com
Fri May 24 11:39:00 GMT 2013
> > Unfortunately, that is a strong point against define_subst in my
> case,
> > since on arm we have more than 400 predicable patterns, of we
> which we
> > might want to modify dozens to perform this cond_exec
> restriction.
> > And creating custom subst-attributes for each one would really
> make
> > things hard to manage/maintain.
> That's definitely a reason:)
>
> > With my proposed modification to define_cond_exec, if we want to
> > restrict the cond_exec variant in the way I described, we only
> add
> > another set_attr to a pattern, without
> > moving around their constraint strings.
> >
> > I'm not sure I see how define_subst could be modified to allow
> for this
> > functionality without impacting the current readability of the md
> > patterns (not to mention the semantics of define_subst itself).
>
> Let me check my understanding of your solution. You suggest to add
> (set_attr "control_var" "yes,no")
> to every define_insn in which we want to disable second alternative
> in
> predicated pattern, right?
Crucially, I want to disable the second alternative in the predicated
pattern on a non-static condition (i.e. a flag, I used
TARGET_RESTRICT_CE in previous examples)
> Then, when cond_exec has processed the initial pattern, we get two
> patterns: in the first one (not-predicated) we have 'predicated'
> attribute set to its default value 'no' and in the second pattern
> (predicated) this attribute is set to 'yes'. Basing on this,
> 'enabled'
> is computed for each pattern. It equals 'yes,yes' for the first one
> and 'yes,no' for the second one.
> So, what you need is to have an attribute to distinguish predicated
> pattern from not-predicated one, correct?
Yes, that's right :)
>
> If that vision is correct, it could be easily done with
> define_subst
> (and without tons of new subst-attributes, as I suggested before:)
> ):
> Just add one subst attribute
> (define_subst_attr "subst_predicated" "ds_predicable" "no" "yes")
> and add it to define_insn pattern:
> (define_insn ...
> [(set_attr "predicable" "yes")
> (set_attr "control_attr" "yes,<subst_predicated>")])
So, "predicable" only has some implicit meaning when define_cond_exec is
used, so we might as well remove that (set_attr "predicable" ...) if we
are going to replace define_cond_exec with a define_subst.
> I think that'll do the trick.
Almost, there is one problem however. What if I want the second
alternative to never be predicated?
The purpose of control_attr in my scheme is to disable the cond_exec
version when a particular flag is set (I used TARGET_RESTRICT_CE as an
example earlier in this thread), not to disable predication of
that alternative forever, that's what "predicable" is for.
As things stand now, if "predicable" is set to "no" for a particular
alternative, the value of control_attr is irrelevant, that alternative
will never have a cond_exec version. In your scheme, however,
the presence of <subst_predicated> triggers the creation of cond_exec
variants for all of the alternatives, even the ones that we don't want
to cond_exec.
Another variant of your suggestion would be:
[(set_attr "predicated" "<subst_predicated>")
(set_attr "control_attr" "yes,no")])
which would set "predicated" to "yes" in the cond_exec version and "no"
in the original, thus distinguishing between them. control_attr would
then be used in the definition of "enabled" to disable the
cond_exec version when a certain flag is set. This is what I want, but
again there's a problem with non-predicable alternatives.
What if I also had a 3rd alternative that was not predicable at all?
(set_attr "predicable" "yes,yes,no").
The presence of <subst_predicated> would force the creation of a
cond_exec variant for that alternative and disabling that would require
another attribute like this:
[(set_attr "predicated" "<subst_predicated>")
(set_attr "control_attr" "yes,no,no") <<< 3rd value is irrelevant
(set_attr "predicable_valid" "yes,yes,no")])
and an extra rule in "enabled" that disables an alternative when
"predicated" is "yes" and "predicable_valid" is "no", which again adds
lots of superfluous churn to the md patterns.
(Sorry if this is a bit of a ramble, I'm just trying to find a solution
that will have the least impact on the md patterns :) )
Thanks,
Kyrill
>
> Everything else is remaining as you suggested (I mean proper use of
> 'control_attr' in 'enabled' attribute).
>
>
> > Thanks,
> > Kyrill
> Thanks, Michael
>
>
> On 24 May 2013 14:11, Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> wrote:
> > Hi Michael,
> >
> >> > - What about define_insn_and_split? Currently, we can define
> >> "predicable"
> >> > for a define_insn_and_split,
> >> Yes, you're right. Currently define_subst cannot be applied to
> >> define_insn_and_split. That's not implemented yet because I
> didn't
> >> see
> >> a real usages of define_substs with these (though I'm not saying
> >> nobody uses it) - in absence of use cases I wasn't able to
> design a
> >> proper syntax for it. If you have any ideas of how that could be
> >> done
> >> in a pretty way, please let me know:)
> >>
> >> As for your second concern:
> >> > - What about predication on a per-alternative basis (set_attr
> >> "predicable"
> >> > "yes,no,yes")?
> >> Currently, cond_exec actually could be a more compact way of
> doing
> >> this. But in general, define_subst is able to substitute it (as
> >> Richard said, it's a superset of define_cond_exec). Here is an
> >> example
> >> of how that could be achieved (maybe, it's not optimal in terms
> of
> >> lines of code):
> >> Suppose we have:
> >> (define_insn "arm_load_exclusive<mode>"
> >> [(set (match_operand:SI 0 "s_register_operand" "=r,r")
> >> (zero_extend:SI
> >> (unspec_volatile:NARROW
> >> [(match_operand:NARROW 1 "mem_noofs_operand"
> "Ua,m")]
> >> VUNSPEC_LL)))]
> >> "TARGET_HAVE_LDREXBH"
> >> "ldrex<sync_sfx>%?\t%0, %C1"
> >> [(set_attr "predicable" "yes,no")])
> >> (I added a second alternative to the define_insn from your
> example)
> >>
> >> We could add several subst-attributes, as following:
> >> (define_subst_attr "constraint_operand1" "ds_predicable" "=r,r"
> >> "=r")
> >> (define_subst_attr "constraint_operand2" "ds_predicable" "=Ua,m"
> >> "Ua")
> >> And then rewrite the original pattern:
> >> (define_insn "arm_load_exclusive<mode>"
> >> [(set (match_operand:SI 0 "s_register_operand"
> >> "<constraint_operand1>")
> >> (zero_extend:SI
> >> (unspec_volatile:NARROW
> >> [(match_operand:NARROW 1 "mem_noofs_operand"
> >> "<constraint_operand2>")]
> >> VUNSPEC_LL)))]
> >> "TARGET_HAVE_LDREXBH"
> >> "ldrex<sync_sfx>%?\t%0, %C1"
> >> []) ;; We don't need this attr for define_subst
> >>
> >> Define_subst (I didn't copy it here) will expand this to the
> next
> >> two patterns:
> >> ;; First pattern (exactly like original), define_subst not-
> applied
> >> (define_insn "arm_load_exclusive<mode>"
> >> [(set (match_operand:SI 0 "s_register_operand" "=r,r")
> >> (zero_extend:SI
> >> (unspec_volatile:NARROW
> >> [(match_operand:NARROW 1 "mem_noofs_operand"
> "Ua,m")]
> >> VUNSPEC_LL)))]
> >> "TARGET_HAVE_LDREXBH"
> >> "ldrex<sync_sfx>%?\t%0, %C1"
> >> [])
> >> ;; Second pattern, with applied define_subst
> >> (define_insn "arm_load_exclusive<mode>"
> >> [(cond_exec
> >> (match_operator 1 "arm_comparison_operator"
> >> [(match_operand 2 "cc_register" "")
> >> (const_int 0)])
> >> (set
> >> (match_operand:SI 0 "s_register_operand" "=r")
> >> (zero_extend:SI
> >> (unspec_volatile:NARROW
> >> [(match_operand:NARROW 1 "mem_noofs_operand" "m")]
> >> VUNSPEC_LL)))]
> >> "TARGET_HAVE_LDREXBH"
> >> "ldrex<sync_sfx>%?\t%0, %C1"
> >> [])
> >>
> >> So, the main idea here is to control how many and what
> alternatives
> >> which pattern would have - and define_subst allows to do that.
> >>
> >> The only problem here is that we might need many subst-
> attributes.
> >
> > Thanks for the explanation.
> >
> > Unfortunately, that is a strong point against define_subst in my
> case,
> > since on arm we have more than 400 predicable patterns, of we
> which we
> > might want to modify dozens to perform this cond_exec
> restriction.
> > And creating custom subst-attributes for each one would really
> make
> > things hard to manage/maintain.
> >
> >> But I think that problem could also be solved if, as Richard
> >> suggested, define_cond_exec would be expanded in gensupport - we
> >> might generate needed attributes there.
> >
> > So, when a subst-attribute is encountered in a pattern, a
> substitute is
> > triggered for the whole pattern (in our case, the cond_exec
> version is
> > created). This includes
> > the alternatives that we might not want predicated. Creating
> > subst-attributes for each operands' constraint string seems like
> a bit
> > of a hack to me,
> > considering that we have a define_cond_exec construct used
> specifically
> > for creating the cond_exec variants by just setting the
> "predicable"
> > attribute each alternative
> > i.e. not touching the patterns themselves.
> >
> > With my proposed modification to define_cond_exec, if we want to
> > restrict the cond_exec variant in the way I described, we only
> add
> > another set_attr to a pattern, without
> > moving around their constraint strings.
> >
> > I'm not sure I see how define_subst could be modified to allow
> for this
> > functionality without impacting the current readability of the md
> > patterns (not to mention the semantics of define_subst itself).
> >
> >> Thus, define_cond_exec would be a
> >> kind of 'syntax sugar' for such cases.
> >
> > From what I can see, define_cond_exec has some extra machinery to
> allow
> > for per-alternative predication (with ce_enabled and
> nonce_enabled) that
> > I don't think can/should
> > be easily replicated in define_subst.
> >
> > According to the documentation, define_subst was created to
> facilitate
> > simple transformations between RTL templates. Seems to me that
> using it
> > to play with constraint alternatives in a fine-grained way, while
> > possible, is not really its purpose.
> >
> > Thanks,
> > Kyrill
> >
> >>
> >> Thanks, Michael
> >>
> >> On 23 May 2013 20:53, Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> >> wrote:
> >> > 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.
> >> >
> >> >
> >> >
> >> >
> >>
> >>
> >>
> >> --
> >> ---
> >> Best regards,
> >> Michael V. Zolotukhin,
> >> Software Engineer
> >> Intel Corporation.
> >
> >
> >
> >
>
>
>
> --
> ---
> Best regards,
> Michael V. Zolotukhin,
> Software Engineer
> Intel Corporation.
More information about the Gcc-patches
mailing list