> - 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.
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. Thus, define_cond_exec would be a
kind of 'syntax sugar' for such cases.
Thanks, Michael
On 23 May 2013 20:53, Kyrylo Tkachov <[email protected]> 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 <[email protected]> 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.
