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.tkac...@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.
