> - 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 <kyrylo.tkac...@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.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. > > > > -- --- Best regards, Michael V. Zolotukhin, Software Engineer Intel Corporation.