Re: [PATCH][gensupport] Add optional attributes field to define_cond_exec

[Michael Zolotukhin <michael dot v dot zolotukhin at gmail dot com>]

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

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")])

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

Hope these examples could help.

Thanks, Michael

On 24 May 2013 15:39, Kyrylo Tkachov <kyrylo.tkachov@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.