> > As things stand now, if "predicable" is set to "no" for a
> particular
> > alternative, the value of control_attr is irrelevant, that
> alternative
> > will never have a cond_exec version. In your scheme, however,
> > the presence of <subst_predicated> triggers the creation of
> cond_exec
> > variants for all of the alternatives, even the ones that we don't
> want
> > to cond_exec.
> Well, that's not quite right. Internally, define_cond_exec works
> pretty similar to define_subst. It can't be applied to one
> alternative
> and not applied to another - it works on the entire pattern. What
> it
> does to distinguish alternatives basing on 'predicable' attribute
> is
> to properly set attribute 'ce_enabled'.
Fair enough. What I meant is that conceptually, it's like there doesn't
exist a cond_exec variant when "predicable" is set to "no".
>
> Here is a small example (you could try it yourself by invoking
> genmddump which is located in build directory):
> -----EXAMPLE 1-----
> (define_attr "predicable" "no,yes" (const_string "no"))
> (define_insn "aaa"
> [(set (match_operand:SI 0 "register_operand" "=r,m,x")
> (match_operand:SI 1 "register_operand" "r,m,x"))]
> ""
> "add %0 %1"
> [(set_attr "predicable" "yes,no,yes")])
>
> (define_cond_exec
> [(match_operator 0 "arm_comparison_operator"
> [(match_operand 1 "cc_register" "")
> (const_int 0)])]
> "TARGET_32BIT"
> "")
> ----- END OF EXAMPLE 1-----
>
> And here is what the compiler has after expanding all patterns (it
> is
> output of genmddump):
> ;; built-in: -1
> (define_attr ("nonce_enabled") ("no,yes") (const_string ("yes")))
> ;; built-in: -1
> (define_attr ("ce_enabled") ("no,yes") (const_string ("yes")))
> ;; a.md: 1
> (define_attr ("predicable") ("no,yes") (const_string ("no")))
> ;; a.md: 3
> (define_insn ("aaa")
> [ (set (match_operand:SI 0 ("register_operand") ("=r,m,x"))
> (match_operand:SI 1 ("register_operand") ("r,m,x")))
> ] ("") ("add %0 %1")
> [ (set_attr ("predicable") ("yes,no,yes")) ])
>
> ;; a.md: 3
> (define_insn ("*p aaa")
> [ (cond_exec (match_operator 2 ("arm_comparison_operator")
> [(match_operand 3 ("cc_register") (""))
> (const_int 0 [0]) ])
> (set (match_operand:SI 0 ("register_operand")
> ("=r,m,x"))
> (match_operand:SI 1 ("register_operand")
> ("r,m,x"))))
> ] ("TARGET_32BIT") ("add %0 %1")
> [ (set_attr ("ce_enabled") ("yes,no,yes")) ])
>
> As you might see, it doesn't distinguish alternatives at all - it
> just
> fills 'ce_enabled' attribute with proper values.
> Here is a second example, which is actually pretty similar to the
> first one, but it's done with define_subst:
> -----EXAMPLE 2-----
> (define_subst_attr "at" "ce_subst" "yes" "no")
>
> (define_insn "aaa"
> [(set (match_operand:SI 0 "register_operand" "=r,m,x")
> (match_operand:SI 1 "register_operand" "r,m,x"))]
> ""
> "add %0 %1"
> [(set_attr "ce_enabled" "yes,<at>,yes")])
Isn't ce_enabled an internal, implicit attribute? I don't think it was
meant to be directly manipulated by the MD patterns.
It seems like a hack around the canonical mechanism we have for defining
conditional execution (by way of the "predicable" attribute).
Presumably, in order to disable the cond_exec variant dynamically
(alternative 3 in your example)
I'd have to add:
(define_subst_attr "subst_predicated" "ce_subst" "no" "yes")
and then in the "aaa" define_insn add:
(set_attr "predicated" "<subst_predicated>")
(set_attr "control_attr" "yes,yes,no")
and manipulate the "enabled" rules considering the dynamic condition,
"predicated" and "control_attr".
This has the disadvantages of:
- Having to copy (set_attr "predicated" "<subst_predicated>") in all
patterns that are predicable.
- Using the internal ce_enabled attribute instead of the canonical
"predicable".
- Still doesn't work for define_insn_and_split ;)
>
> (define_subst "ce_subst"
> [(match_operand 0)]
> "TARGET_32BIT"
> [(cond_exec (match_operator 1 "arm_comparison_operator"
> [(match_operand 2 "cc_register" "")
> (const_int 0)])
> (match_dup 0))])
> -----END OF EXAMPLE 2-----
>
> Here is what compiler has after expanding patterns:
> ;; c.md: 1
> (define_attr ("ce_subst") ("no,yes") (const_string ("no")))
> ;; c.md: 3
> (define_insn ("aaa")
> [ (set (match_operand:SI 0 ("register_operand") ("=r,m,x"))
> (match_operand:SI 1 ("register_operand") ("r,m,x")))
> ] ("") ("add %0 %1")
> [ (set_attr ("ce_enabled") ("yes,yes,yes")) ])
>
> ;; c.md: 3
> (define_insn ("aaa")
> [(cond_exec (match_operator 2 ("arm_comparison_operator")
> [(match_operand 3 ("cc_register") (""))
> (const_int 0 [0])])
> (set (match_operand:SI 0 ("register_operand")
> ("=r,m,x"))
> (match_operand:SI 1 ("register_operand")
> ("r,m,x"))))
> ] ("TARGET_32BIT") ("add %0 %1")
> [(set_attr ("ce_enabled") ("yes,no,yes"))
> (set_attr ("ce_subst") ("no")) ])
>
> You might notice that the output is almost the same (actually, all
> differences could be eliminated except adding new attribute by
> subst
> to substed-pattern). So currently I don't see why define_subst
> can't
> by used instead of define_cond_exec in your case.
I agree that it is possible to use define_subst (apart from the fact
that it doesn't support define_insn_and_split yet). It's just that I
think it looks hacky and makes the patterns look
less readable if the reader has to remember that predication is implicit
when a subst_attr is encountered that is tied to that particular
define_subst.
This will be exacerbated even further if we decide to use other
instances of define_subst for other purposes. The reader would need to
track down the define_subst
corresponding to the subst_attr he/she encountered to figure out whether
it's the ce_subst one. Contrast this with just using "predicable" which
is known and documented to create a cond_exec variant.
>
> Hope these examples could help.
Thanks,
Kyrill
>
> Thanks, Michael
>
> On 24 May 2013 15:39, Kyrylo Tkachov <kyrylo.tkac...@arm.com>
> wrote:
> >> > 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?
> >
> > Crucially, I want to disable the second alternative in the
> predicated
> > pattern on a non-static condition (i.e. a flag, I used
> > TARGET_RESTRICT_CE in previous examples)
> >
> >> 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?
> >
> > Yes, that's right :)
> >
> >>
> >> 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>")])
> >
> > So, "predicable" only has some implicit meaning when
> define_cond_exec is
> > used, so we might as well remove that (set_attr "predicable" ...)
> if we
> > are going to replace define_cond_exec with a define_subst.
> >
> >> I think that'll do the trick.
> >
> > Almost, there is one problem however. What if I want the second
> > alternative to never be predicated?
> > The purpose of control_attr in my scheme is to disable the
> cond_exec
> > version when a particular flag is set (I used TARGET_RESTRICT_CE
> as an
> > example earlier in this thread), not to disable predication of
> > that alternative forever, that's what "predicable" is for.
> >
> > As things stand now, if "predicable" is set to "no" for a
> particular
> > alternative, the value of control_attr is irrelevant, that
> alternative
> > will never have a cond_exec version. In your scheme, however,
> > the presence of <subst_predicated> triggers the creation of
> cond_exec
> > variants for all of the alternatives, even the ones that we don't
> want
> > to cond_exec.
> >
> > Another variant of your suggestion would be:
> > [(set_attr "predicated" "<subst_predicated>")
> > (set_attr "control_attr" "yes,no")])
> >
> > which would set "predicated" to "yes" in the cond_exec version
> and "no"
> > in the original, thus distinguishing between them. control_attr
> would
> > then be used in the definition of "enabled" to disable the
> > cond_exec version when a certain flag is set. This is what I
> want, but
> > again there's a problem with non-predicable alternatives.
> > What if I also had a 3rd alternative that was not predicable at
> all?
> > (set_attr "predicable" "yes,yes,no").
> > The presence of <subst_predicated> would force the creation of a
> > cond_exec variant for that alternative and disabling that would
> require
> > another attribute like this:
> >
> > [(set_attr "predicated" "<subst_predicated>")
> > (set_attr "control_attr" "yes,no,no") <<< 3rd value is
> irrelevant
> > (set_attr "predicable_valid" "yes,yes,no")])
> >
> > and an extra rule in "enabled" that disables an alternative when
> > "predicated" is "yes" and "predicable_valid" is "no", which again
> adds
> > lots of superfluous churn to the md patterns.
> >
> > (Sorry if this is a bit of a ramble, I'm just trying to find a
> solution
> > that will have the least impact on the md patterns :) )
> > Thanks,
> > Kyrill
> >
> >>
> >> Everything else is remaining as you suggested (I mean proper use
> of
> >> 'control_attr' in 'enabled' attribute).
> >>
> >>
> >> > Thanks,
> >> > Kyrill
> >> Thanks, Michael
> >>
> >>
>
>
>
> --
> ---
> Best regards,
> Michael V. Zolotukhin,
> Software Engineer
> Intel Corporation.
