> 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.
It looks hacky indeed, but afaiu, current define_cond_exec doesn't
satisfy your needs as well - you want to add something to it, right?
So I thought that probably it's better to consider using define_subst
(as a possible replacement of define_cond_exec in some future) - and
maybe adjusting it to your needs and making it look less 'hacky', than
changing define_cond_exec.
Anyway, points you mentioned are quite reasonable, so it's all up to you:)
Thanks, Michael
On 24 May 2013 19:22, Kyrylo Tkachov <kyrylo.tkac...@arm.com> wrote:
>> > 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.
>
>
>
>
--
---
Best regards,
Michael V. Zolotukhin,
Software Engineer
Intel Corporation.
