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