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

[Michael Zolotukhin]

> 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?
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?

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>")])
I think that'll do the trick.

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.