Re: [PATCH 1/2]middle-end: RFC: On expansion of conditional branches, give hint if argument is a truth type to backend

[Richard Sandiford <richard.sandiford@arm.com>]

Tamar Christina <Tamar.Christina@arm.com> writes:
>> Maybe the target could use (subreg:SI (reg:BI ...)) as argument. Heh.
>
> But then I'd still need to change the expansion code. I suppose this could
> prevent the issue with changes to code on other targets. 
>
>> > > We have undocumented addcc, negcc, etc. patterns, should we have aandcc
> pattern for this indicating support for andcc + jump as opposedto cmpcc + jump?
>> >
>> > This could work yeah. I didn't know these existed.
>
>> Ah, so they are conditional add, not add setting CC, so andcc wouldn't
>> be appropriate.
>
>> So I'm not sure how we'd handle such situation - maybe looking at
>> REG_DECL and recognizing a _Bool PARM_DECL is OK?
>
> I have a slight suspicion that Richard Sandiford would likely reject this
> though..

Good guess :-P  We shouldn't rely on something like that for correctness.

Would it help if we promoted the test-and-branch instructions to optabs,
alongside cbranch?  The jump expanders could then target it directly.

IMO that'd be a reasonable thing to do if it does help.  It's a relatively
common operation, especially on CISCy targets.

Thanks,
Richard

> The additional AND seemed less hacky as it's just communicating range.
>
> I still need to also figure out which representation of bool is being used,
> because only the 0-1 variant works. Is there a way to check that?
>
> Thanks, 
> Tamar. 
>
> -------------------------------------------------------------------------------
> From: Richard Biener <rguenther@suse.de>
> Sent: Monday, September 26, 2022 12:32 PM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; nd <nd@arm.com>;
> jeffreyalaw@gmail.com <jeffreyalaw@gmail.com>; Richard Sandiford
> <Richard.Sandiford@arm.com>
> Subject: Re: [PATCH 1/2]middle-end: RFC: On expansion of conditional branches,
> give hint if argument is a truth type to backend
>  
> On Mon, 26 Sep 2022, Tamar Christina wrote:
>
>> > > This pattern occurs more than 120,000 times in SPECCPU 2017 and so is
> quite common.
>>
>> > How does this help a target?
>>
>> So the idea is to communicate that only the bottom bit of the value is
> relevant and not the entire value itself.
>>
>> > Why does RTL nonzerop bits not recover thisinformation and the desired
> optimization done later during combinefor example?
>>
>> I'm not sure it works here. We (AArch64) don't have QImode integer registers,
> so our apcs says that the top bits of the 32-bit registers it's passed in are
> undefined.
>>
>> We have to zero extend the value first if we really want it as an 8-bit
> value. So the problem is if you e.g. Pass a boolean as an argument of a
> function I don't think nonzero bits will return anything useful.
>>
>> > Why's a SImode compare not OK if there's no QImode compare?
>>
>> The mode then becomes irrelevant because we're telling the backend that only
> a single bit matters. And we have instructions to test and branch on the value
> of a single bit. See https://gcc.gnu.org/pipermail/gcc-patches/2022-September/
> 602090.html for the testcases
>
> Maybe the target could use (subreg:SI (reg:BI ...)) as argument.  Heh.
>
>> > We have undocumented addcc, negcc, etc. patterns, should we have aandcc
> pattern for this indicating support for andcc + jump as opposedto cmpcc + jump?
>>
>> This could work yeah. I didn't know these existed.
>
> Ah, so they are conditional add, not add setting CC, so andcc wouldn't
> be appropriate.
>
> So I'm not sure how we'd handle such situation - maybe looking at
> REG_DECL and recognizing a _Bool PARM_DECL is OK?
>
> Richard.
>
>> > So - what's the target and what's a testcase?
>>
>> See https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602090.html :)
>>
>> Thanks,
>> Tamar
>>
>> ________________________________
>> From: Richard Biener <rguenther@suse.de>
>> Sent: Monday, September 26, 2022 11:35 AM
>> To: Tamar Christina <Tamar.Christina@arm.com>
>> Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; nd <nd@arm.com>;
> jeffreyalaw@gmail.com <jeffreyalaw@gmail.com>; Richard Sandiford
> <Richard.Sandiford@arm.com>
>> Subject: Re: [PATCH 1/2]middle-end: RFC: On expansion of conditional
> branches, give hint if argument is a truth type to backend
>>
>> On Fri, 23 Sep 2022, Tamar Christina wrote:
>>
>> > Hi All,
>> >
>> > This is an RFC to figure out how to deal with targets that don't have
> native
>> > comparisons against QImode values.
>> >
>> > Booleans, at least in C99 and higher are 0-1 valued.  This means that we
> only
>> > really need to test a single bit.  However in RTL we no longer have this
>> > information available and just have an SImode value (due to the promotion
> of
>> > QImode to SImode).
>> >
>> > This RFC fixes it by emitting an explicit & 1 during the expansion of the
>> > conditional branch.
>> >
>> > However it's unlikely that we want to do this unconditionally.  Most
> targets
>> > I've tested seem to have harmless code changes, like x86 changes from testb
> to
>> > andl, $1.
>> >
>> > So I have two questions:
>> >
>> > 1. Should I limit this behind a target macro? Or should I just leave it for
> all
>> >    targets and deal with the fallout.
>> > 2. How can I tell whether the C99 0-1 values bools are being used or the
> older
>> >    0, non-0 variant?
>> >
>> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>> > However there are some benign codegen changes on x86, testb changed to andl
> $1.
>> >
>> > This pattern occurs more than 120,000 times in SPECCPU 2017 and so is quite
> common.
>>
>> How does this help a target?  Why does RTL nonzerop bits not recover this
>> information and the desired optimization done later during combine
>> for example?  Why's a SImode compare not OK if there's no QImode compare?
>> We have undocumented addcc, negcc, etc. patterns, should we have a
>> andcc pattern for this indicating support for andcc + jump as opposed
>> to cmpcc + jump?
>>
>> So - what's the target and what's a testcase?
>>
>> Thanks,
>> Richard.
>>
>> > Thanks,
>> > Tamar
>> >
>> > gcc/ChangeLog:
>> >
>> >        * tree.h (tree_zero_one_valued_p): New.
>> >        * dojump.cc (do_jump): Add & 1 if truth type.
>> >
>> > --- inline copy of patch --
>> > diff --git a/gcc/dojump.cc b/gcc/dojump.cc
>> > index
> 2af0cd1aca3b6af13d5d8799094ee93f18022296..8eaf1be49cd12298e61c6946ae79ca9de6197864
> 100644
>> > --- a/gcc/dojump.cc
>> > +++ b/gcc/dojump.cc
>> > @@ -605,7 +605,17 @@ do_jump (tree exp, rtx_code_label *if_false_label,
>> >        /* Fall through and generate the normal code.  */
>> >      default:
>> >      normal:
>> > -      temp = expand_normal (exp);
>> > +      tree cmp = exp;
>> > +      /* If the expression is a truth type then explicitly generate an & 1
>> > +      to indicate to the target that it's a zero-one values type.  This
>> > +      allows the target to further optimize the comparison should it
>> > +      choose to.  */
>> > +      if (tree_zero_one_valued_p (exp))
>> > +     {
>> > +       type = TREE_TYPE (exp);
>> > +       cmp = build2 (BIT_AND_EXPR, type, exp, build_int_cstu (type, 1));
>> > +     }
>> > +      temp = expand_normal (cmp);
>> >        do_pending_stack_adjust ();
>> >        /* The RTL optimizers prefer comparisons against pseudos.  */
>> >        if (GET_CODE (temp) == SUBREG)
>> > diff --git a/gcc/tree.h b/gcc/tree.h
>> > index
> 8f8a9660c9e0605eb516de194640b8c1b531b798..be3d2dee82f692e81082cf21c878c10f9fe9e1f1
> 100644
>> > --- a/gcc/tree.h
>> > +++ b/gcc/tree.h
>> > @@ -4690,6 +4690,7 @@ extern tree signed_or_unsigned_type_for (int, tree);
>> >  extern tree signed_type_for (tree);
>> >  extern tree unsigned_type_for (tree);
>> >  extern bool is_truth_type_for (tree, tree);
>> > +extern bool tree_zero_one_valued_p (tree);
>> >  extern tree truth_type_for (tree);
>> >  extern tree build_pointer_type_for_mode (tree, machine_mode, bool);
>> >  extern tree build_pointer_type (tree);
>> >
>> >
>> >
>> >
>> >
>>
>> --
>> Richard Biener <rguenther@suse.de>
>> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
>> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
>> HRB 36809 (AG Nuernberg)
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)