RE: [PATCH 1/2]middle-end: Add new tbranch optab to add support for bit-test-and-branch operations

[Tamar Christina <Tamar.Christina@arm.com>]

> -----Original Message-----
> From: Jeff Law <jeffreyalaw@gmail.com>
> Sent: Monday, October 31, 2022 9:16 PM
> To: Tamar Christina <Tamar.Christina@arm.com>; gcc-patches@gcc.gnu.org
> Cc: nd <nd@arm.com>; rguenther@suse.de
> Subject: Re: [PATCH 1/2]middle-end: Add new tbranch optab to add support
> for bit-test-and-branch operations
> 
> 
> On 10/31/22 05:53, Tamar Christina wrote:
> > Hi All,
> >
> > This adds a new test-and-branch optab that can be used to do a conditional
> test
> > of a bit and branch.   This is similar to the cbranch optab but instead can
> > test any arbitrary bit inside the register.
> >
> > This patch recognizes boolean comparisons and single bit mask tests.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >
> > Ok for master?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 	* dojump.cc (do_jump): Pass along value.
> > 	(do_jump_by_parts_greater_rtx): Likewise.
> > 	(do_jump_by_parts_zero_rtx): Likewise.
> > 	(do_jump_by_parts_equality_rtx): Likewise.
> > 	(do_compare_rtx_and_jump): Likewise.
> > 	(do_compare_and_jump): Likewise.
> > 	* dojump.h (do_compare_rtx_and_jump): New.
> > 	* optabs.cc (emit_cmp_and_jump_insn_1): Refactor to take optab
> to check.
> > 	(validate_test_and_branch): New.
> > 	(emit_cmp_and_jump_insns): Optiobally take a value, and when
> value is
> > 	supplied then check if it's suitable for tbranch.
> > 	* optabs.def (tbranch$a4): New.
> > 	* doc/md.texi (tbranch@var{mode}4): Document it.
> > 	* optabs.h (emit_cmp_and_jump_insns):
> > 	* tree.h (tree_zero_one_valued_p): New.
> >
> > --- inline copy of patch --
> > diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi index
> >
> c08691ab4c9a4bfe55ae81e5e228a414d6242d78..f8b32ec12f46d3fb3815f121a1
> 6b
> > 5a8a1819b66a 100644
> > --- a/gcc/doc/md.texi
> > +++ b/gcc/doc/md.texi
> > @@ -6972,6 +6972,13 @@ case, you can and should make operand 1's
> predicate reject some operators
> >   in the @samp{cstore@var{mode}4} pattern, or remove the pattern
> altogether
> >   from the machine description.
> >
> > +@cindex @code{tbranch@var{mode}4} instruction pattern @item
> > +@samp{tbranch@var{mode}4} Conditional branch instruction combined
> > +with a bit test-and-compare instruction. Operand 0 is a comparison
> > +operator.  Operand 1 is the operand of the comparison. Operand 2 is
> > +the bit position of Operand 1 to test.
> > +Operand 3 is the @code{code_label} to jump to.
> 
> Should we refine/document the set of comparison operators allowed?    Is
> operand 1 an arbitrary RTL expression or more limited?  I'm guessing its
> relatively arbitrary given how you've massaged the existing branch-on-bit
> patterns from the aarch backend.

It can be any expression in theory. However in practical terms we usually force
the values to registers before calling the expansion.  My assumption is that this
is for CSE purposes but that's only a guess.

> 
> 
> > +
> > +  if (TREE_CODE (val) != SSA_NAME)
> > +    return false;
> > +
> > +  gimple *def = SSA_NAME_DEF_STMT (val);  if (!is_gimple_assign (def)
> > +      || gimple_assign_rhs_code (def) != BIT_AND_EXPR)
> > +    return false;
> > +
> > +  tree cst = gimple_assign_rhs2 (def);
> > +
> > +  if (!tree_fits_uhwi_p (cst))
> > +    return false;
> > +
> > +  tree op0 = gimple_assign_rhs1 (def);
> > +  if (TREE_CODE (op0) == SSA_NAME)
> > +    {
> > +      def = SSA_NAME_DEF_STMT (op0);
> > +      if (gimple_assign_cast_p (def))
> > +	op0 = gimple_assign_rhs1 (def);
> > +    }
> > +
> > +  wide_int wcst = wi::uhwi (tree_to_uhwi (cst),
> > +			    TYPE_PRECISION (TREE_TYPE (op0)));
> > +  int bitpos;
> > +
> > +  if ((bitpos = wi::exact_log2 (wcst)) == -1)
> > +    return false;
> 
> Do we have enough information lying around from Ranger to avoid the need
> to walk the def-use chain to discover that we're masking off all but one bit?
> 

That's an interesting thought.  I'll try to see if I can figure out how to query
Ranger here.  It would be nice to do so here.

Cheers,
Tamar

> 
> 
> >
> >
> > diff --git a/gcc/tree.h b/gcc/tree.h
> > index
> >
> 8f8a9660c9e0605eb516de194640b8c1b531b798..be3d2dee82f692e81082cf21c
> 878
> > c10f9fe9e1f1 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);
> 
> I don't see a definition of this anywhere.
> 
> 
> jeff
>