RISC-V: Handle "(a & twobits) == singlebit" in branches using Zbs

Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
that can be expressed as bexti + bexti + andn.

gcc/ChangeLog:

	* config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
	Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
	of these tow bits set.
	* config/riscv/predicates.md (const_twobits_operand): New predicate.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/zbs-if_then_else-01.c: New test.

Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---

 gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
 gcc/config/riscv/predicates.md                |  5 +++
 .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
 3 files changed, 67 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c

On 11/13/22 13:48, Philipp Tomsich wrote:
> Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> that can be expressed as bexti + bexti + andn.
>
> gcc/ChangeLog:
>
> 	* config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
> 	Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
> 	of these tow bits set.
> 	* config/riscv/predicates.md (const_twobits_operand): New predicate.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/riscv/zbs-if_then_else-01.c: New test.

s/tow/two/ in the ChangeLog.





>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> ---
>
>   gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
>   gcc/config/riscv/predicates.md                |  5 +++
>   .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
>   3 files changed, 67 insertions(+)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
>
> diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> index 7a8f4e35880..2cea394671f 100644
> --- a/gcc/config/riscv/bitmanip.md
> +++ b/gcc/config/riscv/bitmanip.md
> @@ -690,3 +690,45 @@
>     "TARGET_ZBS"
>     [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
>      (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> +
> +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> +  [(set (pc)
> +	(if_then_else (match_operator 1 "equality_operator"
> +		       [(and:X (match_operand:X 2 "register_operand" "r")
> +			       (match_operand:X 3 "const_twobits_operand" "i"))
> +			(match_operand:X 4 "single_bit_mask_operand" "i")])
> +	 (label_ref (match_operand 0 "" ""))
> +	 (pc)))
> +   (clobber (match_scratch:X 5 "=&r"))
> +   (clobber (match_scratch:X 6 "=&r"))]
> +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"
> +  "#"
> +  "&& reload_completed"
> +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> +				      (const_int 1)
> +				      (match_dup 8)))
> +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> +				      (const_int 1)
> +				      (match_dup 9)))
> +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> +			   (label_ref (match_dup 0))
> +			   (pc)))]
> +{
> +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> +
> +   /* Make sure that the reference value has one of the bits of the mask set */
> +   if ((twobits_mask & singlebit_mask) == 0)
> +      FAIL;

This fails the split, but in the event this scenario occurs we still 
would up with an ICE as the output template requires splitting.  Don't 
we need to have this be part of the pattern's condition instead so that 
it never matches in that case?


ISTM we should probably have a test to cover this scenario.



jeff

On Thu, 17 Nov 2022 at 15:58, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
> On 11/13/22 13:48, Philipp Tomsich wrote:
> > Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> > that can be expressed as bexti + bexti + andn.
> >
> > gcc/ChangeLog:
> >
> >       * config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
> >       Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
> >       of these tow bits set.
> >       * config/riscv/predicates.md (const_twobits_operand): New predicate.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       * gcc.target/riscv/zbs-if_then_else-01.c: New test.
>
> s/tow/two/ in the ChangeLog.
>
>
>
>
>
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> >   gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
> >   gcc/config/riscv/predicates.md                |  5 +++
> >   .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
> >   3 files changed, 67 insertions(+)
> >   create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> >
> > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > index 7a8f4e35880..2cea394671f 100644
> > --- a/gcc/config/riscv/bitmanip.md
> > +++ b/gcc/config/riscv/bitmanip.md
> > @@ -690,3 +690,45 @@
> >     "TARGET_ZBS"
> >     [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
> >      (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> > +
> > +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> > +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> > +  [(set (pc)
> > +     (if_then_else (match_operator 1 "equality_operator"
> > +                    [(and:X (match_operand:X 2 "register_operand" "r")
> > +                            (match_operand:X 3 "const_twobits_operand" "i"))
> > +                     (match_operand:X 4 "single_bit_mask_operand" "i")])
> > +      (label_ref (match_operand 0 "" ""))
> > +      (pc)))
> > +   (clobber (match_scratch:X 5 "=&r"))
> > +   (clobber (match_scratch:X 6 "=&r"))]
> > +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"
> > +  "#"
> > +  "&& reload_completed"
> > +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> > +                                   (const_int 1)
> > +                                   (match_dup 8)))
> > +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> > +                                   (const_int 1)
> > +                                   (match_dup 9)))
> > +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> > +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> > +                        (label_ref (match_dup 0))
> > +                        (pc)))]
> > +{
> > +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> > +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> > +
> > +   /* Make sure that the reference value has one of the bits of the mask set */
> > +   if ((twobits_mask & singlebit_mask) == 0)
> > +      FAIL;
>
> This fails the split, but in the event this scenario occurs we still
> would up with an ICE as the output template requires splitting.  Don't
> we need to have this be part of the pattern's condition instead so that
> it never matches in that case?

This serves as an assertion only, as that case is non-sensical and
will be optimized away by earlier passes (as "a & C == T" with C and T
sharing no bits will always be false).
IFAIK the preceding transforms should always clean such a check up,
but we can't exclude the possibility that with enough command line
overrides and params we might see such a non-sensical test making it
all the way to the backend.

What would you recommend? Adding this to the pattern's condition feels
a bit redundant.
In fact, I am leaning towards hiding the !SMALL_OPERAND check in yet
another predicate that combines const_twobits_operand with a
match_test for !SMALL_OPERAND.

> ISTM we should probably have a test to cover this scenario.
>
>
>
> jeff
>
>

On 11/17/22 08:12, Philipp Tomsich wrote:
>
> This serves as an assertion only, as that case is non-sensical and
> will be optimized away by earlier passes (as "a & C == T" with C and T
> sharing no bits will always be false).
> IFAIK the preceding transforms should always clean such a check up,
> but we can't exclude the possibility that with enough command line
> overrides and params we might see such a non-sensical test making it
> all the way to the backend.

Good!  I was thinking in the back of my mind that the no-sharing-bits 
case should have been handled in the generic optimizers.  Thanks for 
clarifying.


>
> What would you recommend? Adding this to the pattern's condition feels
> a bit redundant.

We can leave it in the splitter.


> In fact, I am leaning towards hiding the !SMALL_OPERAND check in yet
> another predicate that combines const_twobits_operand with a
> match_test for !SMALL_OPERAND.

Sure.

jeff

On Thu, 17 Nov 2022 at 17:39, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
> On 11/17/22 08:12, Philipp Tomsich wrote:
> >
> > This serves as an assertion only, as that case is non-sensical and
> > will be optimized away by earlier passes (as "a & C == T" with C and T
> > sharing no bits will always be false).
> > IFAIK the preceding transforms should always clean such a check up,
> > but we can't exclude the possibility that with enough command line
> > overrides and params we might see such a non-sensical test making it
> > all the way to the backend.
>
> Good!  I was thinking in the back of my mind that the no-sharing-bits
> case should have been handled in the generic optimizers.  Thanks for
> clarifying.
>
>
> >
> > What would you recommend? Adding this to the pattern's condition feels
> > a bit redundant.
>
> We can leave it in the splitter.
>
>
> > In fact, I am leaning towards hiding the !SMALL_OPERAND check in yet
> > another predicate that combines const_twobits_operand with a
> > match_test for !SMALL_OPERAND.

I'll send a v2 with this cleaned up (and look into clarifying things
around the FAIL).

Philipp.

On Sun, Nov 13, 2022 at 12:51 PM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> that can be expressed as bexti + bexti + andn.

Can't you also handle if ((a & twobits) == 0) case doing a similar thing.
That is:
two bexti + and and then compare against zero which is exactly the
same # of instructions as the above case.


>
> gcc/ChangeLog:
>
>         * config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
>         Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
>         of these tow bits set.
>         * config/riscv/predicates.md (const_twobits_operand): New predicate.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/zbs-if_then_else-01.c: New test.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> ---
>
>  gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
>  gcc/config/riscv/predicates.md                |  5 +++
>  .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
>  3 files changed, 67 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
>
> diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> index 7a8f4e35880..2cea394671f 100644
> --- a/gcc/config/riscv/bitmanip.md
> +++ b/gcc/config/riscv/bitmanip.md
> @@ -690,3 +690,45 @@
>    "TARGET_ZBS"
>    [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
>     (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> +
> +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> +  [(set (pc)
> +       (if_then_else (match_operator 1 "equality_operator"
> +                      [(and:X (match_operand:X 2 "register_operand" "r")
> +                              (match_operand:X 3 "const_twobits_operand" "i"))
> +                       (match_operand:X 4 "single_bit_mask_operand" "i")])
> +        (label_ref (match_operand 0 "" ""))
> +        (pc)))
> +   (clobber (match_scratch:X 5 "=&r"))
> +   (clobber (match_scratch:X 6 "=&r"))]
> +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"
> +  "#"
> +  "&& reload_completed"
> +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> +                                     (const_int 1)
> +                                     (match_dup 8)))
> +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> +                                     (const_int 1)
> +                                     (match_dup 9)))
> +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> +                          (label_ref (match_dup 0))
> +                          (pc)))]
> +{
> +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> +
> +   /* Make sure that the reference value has one of the bits of the mask set */
> +   if ((twobits_mask & singlebit_mask) == 0)
> +      FAIL;
> +
> +   int setbit = ctz_hwi (singlebit_mask);
> +   int clearbit = ctz_hwi (twobits_mask & ~singlebit_mask);
> +
> +   operands[1] = gen_rtx_fmt_ee (GET_CODE (operands[1]) == NE ? EQ : NE,
> +                                <X:MODE>mode, operands[6], GEN_INT(0));
> +
> +   operands[8] = GEN_INT (setbit);
> +   operands[9] = GEN_INT (clearbit);
> +})
> diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
> index 490bff688a7..6e34829a59b 100644
> --- a/gcc/config/riscv/predicates.md
> +++ b/gcc/config/riscv/predicates.md
> @@ -321,6 +321,11 @@
>    (and (match_code "const_int")
>         (match_test "popcount_hwi (~UINTVAL (op)) == 2")))
>
> +;; A CONST_INT operand that has exactly two bits set.
> +(define_predicate "const_twobits_operand"
> +  (and (match_code "const_int")
> +       (match_test "popcount_hwi (UINTVAL (op)) == 2")))
> +
>  ;; A CONST_INT operand that fits into the unsigned half of a
>  ;; signed-immediate after the top bit has been cleared.
>  (define_predicate "uimm_extra_bit_operand"
> diff --git a/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> new file mode 100644
> index 00000000000..d249a841ff9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc_zbb_zbs -mabi=lp64" } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */

It would be useful to add a rv32 testcase too.

Thanks,
Andrew Pinski

> +
> +void g();
> +
> +void f1 (long a)
> +{
> +  if ((a & ((1ul << 33) | (1 << 4))) == (1ul << 33))
> +    g();
> +}
> +
> +void f2 (long a)
> +{
> +  if ((a & 0x12) == 0x10)
> +    g();
> +}
> +
> +/* { dg-final { scan-assembler-times "bexti\t" 2 } } */
> +/* { dg-final { scan-assembler-times "andn\t" 1 } } */
> --
> 2.34.1
>

On Thu, Nov 17, 2022 at 10:25 AM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Sun, Nov 13, 2022 at 12:51 PM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> > that can be expressed as bexti + bexti + andn.
>
> Can't you also handle if ((a & twobits) == 0) case doing a similar thing.
> That is:
> two bexti + and and then compare against zero which is exactly the
> same # of instructions as the above case.
>
>
> >
> > gcc/ChangeLog:
> >
> >         * config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
> >         Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
> >         of these tow bits set.
> >         * config/riscv/predicates.md (const_twobits_operand): New predicate.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.target/riscv/zbs-if_then_else-01.c: New test.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> >  gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
> >  gcc/config/riscv/predicates.md                |  5 +++
> >  .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
> >  3 files changed, 67 insertions(+)
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> >
> > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > index 7a8f4e35880..2cea394671f 100644
> > --- a/gcc/config/riscv/bitmanip.md
> > +++ b/gcc/config/riscv/bitmanip.md
> > @@ -690,3 +690,45 @@
> >    "TARGET_ZBS"
> >    [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
> >     (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> > +
> > +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> > +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> > +  [(set (pc)
> > +       (if_then_else (match_operator 1 "equality_operator"
> > +                      [(and:X (match_operand:X 2 "register_operand" "r")
> > +                              (match_operand:X 3 "const_twobits_operand" "i"))
> > +                       (match_operand:X 4 "single_bit_mask_operand" "i")])
> > +        (label_ref (match_operand 0 "" ""))
> > +        (pc)))
> > +   (clobber (match_scratch:X 5 "=&r"))
> > +   (clobber (match_scratch:X 6 "=&r"))]
> > +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"

Is there a reason why you can't do this at expand time? I think there
are recent patches floating around which is supposed to help with that
case and the RISCV backend just needs to plug into that infrastructure
too.

Thanks,
Andrew Pinski

> > +  "#"
> > +  "&& reload_completed"
> > +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> > +                                     (const_int 1)
> > +                                     (match_dup 8)))
> > +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> > +                                     (const_int 1)
> > +                                     (match_dup 9)))
> > +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> > +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> > +                          (label_ref (match_dup 0))
> > +                          (pc)))]
> > +{
> > +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> > +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> > +
> > +   /* Make sure that the reference value has one of the bits of the mask set */
> > +   if ((twobits_mask & singlebit_mask) == 0)
> > +      FAIL;
> > +
> > +   int setbit = ctz_hwi (singlebit_mask);
> > +   int clearbit = ctz_hwi (twobits_mask & ~singlebit_mask);
> > +
> > +   operands[1] = gen_rtx_fmt_ee (GET_CODE (operands[1]) == NE ? EQ : NE,
> > +                                <X:MODE>mode, operands[6], GEN_INT(0));
> > +
> > +   operands[8] = GEN_INT (setbit);
> > +   operands[9] = GEN_INT (clearbit);
> > +})
> > diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
> > index 490bff688a7..6e34829a59b 100644
> > --- a/gcc/config/riscv/predicates.md
> > +++ b/gcc/config/riscv/predicates.md
> > @@ -321,6 +321,11 @@
> >    (and (match_code "const_int")
> >         (match_test "popcount_hwi (~UINTVAL (op)) == 2")))
> >
> > +;; A CONST_INT operand that has exactly two bits set.
> > +(define_predicate "const_twobits_operand"
> > +  (and (match_code "const_int")
> > +       (match_test "popcount_hwi (UINTVAL (op)) == 2")))
> > +
> >  ;; A CONST_INT operand that fits into the unsigned half of a
> >  ;; signed-immediate after the top bit has been cleared.
> >  (define_predicate "uimm_extra_bit_operand"
> > diff --git a/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > new file mode 100644
> > index 00000000000..d249a841ff9
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > @@ -0,0 +1,20 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-march=rv64gc_zbb_zbs -mabi=lp64" } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
>
> It would be useful to add a rv32 testcase too.
>
> Thanks,
> Andrew Pinski
>
> > +
> > +void g();
> > +
> > +void f1 (long a)
> > +{
> > +  if ((a & ((1ul << 33) | (1 << 4))) == (1ul << 33))
> > +    g();
> > +}
> > +
> > +void f2 (long a)
> > +{
> > +  if ((a & 0x12) == 0x10)
> > +    g();
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "bexti\t" 2 } } */
> > +/* { dg-final { scan-assembler-times "andn\t" 1 } } */
> > --
> > 2.34.1
> >

Am I wrong to worry that this will increase dynamic instruction count
when used in a loop?  The obvious code is more efficient when the
constant loads can be hoisted out of a loop.  Or does the cost model
account for this somehow?


On Sun, Nov 13, 2022 at 12:50 PM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> that can be expressed as bexti + bexti + andn.
>
> gcc/ChangeLog:
>
>         * config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
>         Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
>         of these tow bits set.
>         * config/riscv/predicates.md (const_twobits_operand): New predicate.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/zbs-if_then_else-01.c: New test.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> ---
>
>  gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
>  gcc/config/riscv/predicates.md                |  5 +++
>  .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
>  3 files changed, 67 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
>
> diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> index 7a8f4e35880..2cea394671f 100644
> --- a/gcc/config/riscv/bitmanip.md
> +++ b/gcc/config/riscv/bitmanip.md
> @@ -690,3 +690,45 @@
>    "TARGET_ZBS"
>    [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
>     (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> +
> +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> +  [(set (pc)
> +       (if_then_else (match_operator 1 "equality_operator"
> +                      [(and:X (match_operand:X 2 "register_operand" "r")
> +                              (match_operand:X 3 "const_twobits_operand" "i"))
> +                       (match_operand:X 4 "single_bit_mask_operand" "i")])
> +        (label_ref (match_operand 0 "" ""))
> +        (pc)))
> +   (clobber (match_scratch:X 5 "=&r"))
> +   (clobber (match_scratch:X 6 "=&r"))]
> +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"
> +  "#"
> +  "&& reload_completed"
> +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> +                                     (const_int 1)
> +                                     (match_dup 8)))
> +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> +                                     (const_int 1)
> +                                     (match_dup 9)))
> +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> +                          (label_ref (match_dup 0))
> +                          (pc)))]
> +{
> +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> +
> +   /* Make sure that the reference value has one of the bits of the mask set */
> +   if ((twobits_mask & singlebit_mask) == 0)
> +      FAIL;
> +
> +   int setbit = ctz_hwi (singlebit_mask);
> +   int clearbit = ctz_hwi (twobits_mask & ~singlebit_mask);
> +
> +   operands[1] = gen_rtx_fmt_ee (GET_CODE (operands[1]) == NE ? EQ : NE,
> +                                <X:MODE>mode, operands[6], GEN_INT(0));
> +
> +   operands[8] = GEN_INT (setbit);
> +   operands[9] = GEN_INT (clearbit);
> +})
> diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
> index 490bff688a7..6e34829a59b 100644
> --- a/gcc/config/riscv/predicates.md
> +++ b/gcc/config/riscv/predicates.md
> @@ -321,6 +321,11 @@
>    (and (match_code "const_int")
>         (match_test "popcount_hwi (~UINTVAL (op)) == 2")))
>
> +;; A CONST_INT operand that has exactly two bits set.
> +(define_predicate "const_twobits_operand"
> +  (and (match_code "const_int")
> +       (match_test "popcount_hwi (UINTVAL (op)) == 2")))
> +
>  ;; A CONST_INT operand that fits into the unsigned half of a
>  ;; signed-immediate after the top bit has been cleared.
>  (define_predicate "uimm_extra_bit_operand"
> diff --git a/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> new file mode 100644
> index 00000000000..d249a841ff9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc_zbb_zbs -mabi=lp64" } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
> +
> +void g();
> +
> +void f1 (long a)
> +{
> +  if ((a & ((1ul << 33) | (1 << 4))) == (1ul << 33))
> +    g();
> +}
> +
> +void f2 (long a)
> +{
> +  if ((a & 0x12) == 0x10)
> +    g();
> +}
> +
> +/* { dg-final { scan-assembler-times "bexti\t" 2 } } */
> +/* { dg-final { scan-assembler-times "andn\t" 1 } } */
> --
> 2.34.1
>

On Thu, 17 Nov 2022 at 19:33, Andrew Waterman <andrew@sifive.com> wrote:
>
> Am I wrong to worry that this will increase dynamic instruction count
> when used in a loop?  The obvious code is more efficient when the
> constant loads can be hoisted out of a loop.  Or does the cost model
> account for this somehow?

With this change merged, GCC still hoists the constants out of the
loop (just checked with a quick test case).
So the cost model seems correct (whether intentionally or accidentally).

Thanks,
Philipp.

>
>
> On Sun, Nov 13, 2022 at 12:50 PM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> > that can be expressed as bexti + bexti + andn.
> >
> > gcc/ChangeLog:
> >
> >         * config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
> >         Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
> >         of these tow bits set.
> >         * config/riscv/predicates.md (const_twobits_operand): New predicate.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.target/riscv/zbs-if_then_else-01.c: New test.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> >  gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
> >  gcc/config/riscv/predicates.md                |  5 +++
> >  .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
> >  3 files changed, 67 insertions(+)
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> >
> > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > index 7a8f4e35880..2cea394671f 100644
> > --- a/gcc/config/riscv/bitmanip.md
> > +++ b/gcc/config/riscv/bitmanip.md
> > @@ -690,3 +690,45 @@
> >    "TARGET_ZBS"
> >    [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
> >     (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> > +
> > +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> > +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> > +  [(set (pc)
> > +       (if_then_else (match_operator 1 "equality_operator"
> > +                      [(and:X (match_operand:X 2 "register_operand" "r")
> > +                              (match_operand:X 3 "const_twobits_operand" "i"))
> > +                       (match_operand:X 4 "single_bit_mask_operand" "i")])
> > +        (label_ref (match_operand 0 "" ""))
> > +        (pc)))
> > +   (clobber (match_scratch:X 5 "=&r"))
> > +   (clobber (match_scratch:X 6 "=&r"))]
> > +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"
> > +  "#"
> > +  "&& reload_completed"
> > +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> > +                                     (const_int 1)
> > +                                     (match_dup 8)))
> > +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> > +                                     (const_int 1)
> > +                                     (match_dup 9)))
> > +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> > +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> > +                          (label_ref (match_dup 0))
> > +                          (pc)))]
> > +{
> > +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> > +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> > +
> > +   /* Make sure that the reference value has one of the bits of the mask set */
> > +   if ((twobits_mask & singlebit_mask) == 0)
> > +      FAIL;
> > +
> > +   int setbit = ctz_hwi (singlebit_mask);
> > +   int clearbit = ctz_hwi (twobits_mask & ~singlebit_mask);
> > +
> > +   operands[1] = gen_rtx_fmt_ee (GET_CODE (operands[1]) == NE ? EQ : NE,
> > +                                <X:MODE>mode, operands[6], GEN_INT(0));
> > +
> > +   operands[8] = GEN_INT (setbit);
> > +   operands[9] = GEN_INT (clearbit);
> > +})
> > diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
> > index 490bff688a7..6e34829a59b 100644
> > --- a/gcc/config/riscv/predicates.md
> > +++ b/gcc/config/riscv/predicates.md
> > @@ -321,6 +321,11 @@
> >    (and (match_code "const_int")
> >         (match_test "popcount_hwi (~UINTVAL (op)) == 2")))
> >
> > +;; A CONST_INT operand that has exactly two bits set.
> > +(define_predicate "const_twobits_operand"
> > +  (and (match_code "const_int")
> > +       (match_test "popcount_hwi (UINTVAL (op)) == 2")))
> > +
> >  ;; A CONST_INT operand that fits into the unsigned half of a
> >  ;; signed-immediate after the top bit has been cleared.
> >  (define_predicate "uimm_extra_bit_operand"
> > diff --git a/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > new file mode 100644
> > index 00000000000..d249a841ff9
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > @@ -0,0 +1,20 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-march=rv64gc_zbb_zbs -mabi=lp64" } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
> > +
> > +void g();
> > +
> > +void f1 (long a)
> > +{
> > +  if ((a & ((1ul << 33) | (1 << 4))) == (1ul << 33))
> > +    g();
> > +}
> > +
> > +void f2 (long a)
> > +{
> > +  if ((a & 0x12) == 0x10)
> > +    g();
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "bexti\t" 2 } } */
> > +/* { dg-final { scan-assembler-times "andn\t" 1 } } */
> > --
> > 2.34.1
> >

On Thu, Nov 17, 2022 at 10:52 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> On Thu, 17 Nov 2022 at 19:33, Andrew Waterman <andrew@sifive.com> wrote:
> >
> > Am I wrong to worry that this will increase dynamic instruction count
> > when used in a loop?  The obvious code is more efficient when the
> > constant loads can be hoisted out of a loop.  Or does the cost model
> > account for this somehow?
>
> With this change merged, GCC still hoists the constants out of the
> loop (just checked with a quick test case).
> So the cost model seems correct (whether intentionally or accidentally).

Cool, thanks for checking.

>
> Thanks,
> Philipp.
>
> >
> >
> > On Sun, Nov 13, 2022 at 12:50 PM Philipp Tomsich
> > <philipp.tomsich@vrull.eu> wrote:
> > >
> > > Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> > > that can be expressed as bexti + bexti + andn.
> > >
> > > gcc/ChangeLog:
> > >
> > >         * config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
> > >         Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
> > >         of these tow bits set.
> > >         * config/riscv/predicates.md (const_twobits_operand): New predicate.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * gcc.target/riscv/zbs-if_then_else-01.c: New test.
> > >
> > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > > ---
> > >
> > >  gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
> > >  gcc/config/riscv/predicates.md                |  5 +++
> > >  .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
> > >  3 files changed, 67 insertions(+)
> > >  create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > >
> > > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > > index 7a8f4e35880..2cea394671f 100644
> > > --- a/gcc/config/riscv/bitmanip.md
> > > +++ b/gcc/config/riscv/bitmanip.md
> > > @@ -690,3 +690,45 @@
> > >    "TARGET_ZBS"
> > >    [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
> > >     (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> > > +
> > > +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> > > +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> > > +  [(set (pc)
> > > +       (if_then_else (match_operator 1 "equality_operator"
> > > +                      [(and:X (match_operand:X 2 "register_operand" "r")
> > > +                              (match_operand:X 3 "const_twobits_operand" "i"))
> > > +                       (match_operand:X 4 "single_bit_mask_operand" "i")])
> > > +        (label_ref (match_operand 0 "" ""))
> > > +        (pc)))
> > > +   (clobber (match_scratch:X 5 "=&r"))
> > > +   (clobber (match_scratch:X 6 "=&r"))]
> > > +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"
> > > +  "#"
> > > +  "&& reload_completed"
> > > +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> > > +                                     (const_int 1)
> > > +                                     (match_dup 8)))
> > > +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> > > +                                     (const_int 1)
> > > +                                     (match_dup 9)))
> > > +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> > > +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> > > +                          (label_ref (match_dup 0))
> > > +                          (pc)))]
> > > +{
> > > +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> > > +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> > > +
> > > +   /* Make sure that the reference value has one of the bits of the mask set */
> > > +   if ((twobits_mask & singlebit_mask) == 0)
> > > +      FAIL;
> > > +
> > > +   int setbit = ctz_hwi (singlebit_mask);
> > > +   int clearbit = ctz_hwi (twobits_mask & ~singlebit_mask);
> > > +
> > > +   operands[1] = gen_rtx_fmt_ee (GET_CODE (operands[1]) == NE ? EQ : NE,
> > > +                                <X:MODE>mode, operands[6], GEN_INT(0));
> > > +
> > > +   operands[8] = GEN_INT (setbit);
> > > +   operands[9] = GEN_INT (clearbit);
> > > +})
> > > diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
> > > index 490bff688a7..6e34829a59b 100644
> > > --- a/gcc/config/riscv/predicates.md
> > > +++ b/gcc/config/riscv/predicates.md
> > > @@ -321,6 +321,11 @@
> > >    (and (match_code "const_int")
> > >         (match_test "popcount_hwi (~UINTVAL (op)) == 2")))
> > >
> > > +;; A CONST_INT operand that has exactly two bits set.
> > > +(define_predicate "const_twobits_operand"
> > > +  (and (match_code "const_int")
> > > +       (match_test "popcount_hwi (UINTVAL (op)) == 2")))
> > > +
> > >  ;; A CONST_INT operand that fits into the unsigned half of a
> > >  ;; signed-immediate after the top bit has been cleared.
> > >  (define_predicate "uimm_extra_bit_operand"
> > > diff --git a/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > > new file mode 100644
> > > index 00000000000..d249a841ff9
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > > @@ -0,0 +1,20 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-march=rv64gc_zbb_zbs -mabi=lp64" } */
> > > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
> > > +
> > > +void g();
> > > +
> > > +void f1 (long a)
> > > +{
> > > +  if ((a & ((1ul << 33) | (1 << 4))) == (1ul << 33))
> > > +    g();
> > > +}
> > > +
> > > +void f2 (long a)
> > > +{
> > > +  if ((a & 0x12) == 0x10)
> > > +    g();
> > > +}
> > > +
> > > +/* { dg-final { scan-assembler-times "bexti\t" 2 } } */
> > > +/* { dg-final { scan-assembler-times "andn\t" 1 } } */
> > > --
> > > 2.34.1
> > >

On Thu, 17 Nov 2022 at 19:28, Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Thu, Nov 17, 2022 at 10:25 AM Andrew Pinski <pinskia@gmail.com> wrote:
> >
> > On Sun, Nov 13, 2022 at 12:51 PM Philipp Tomsich
> > <philipp.tomsich@vrull.eu> wrote:
> > >
> > > Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> > > that can be expressed as bexti + bexti + andn.
> >
> > Can't you also handle if ((a & twobits) == 0) case doing a similar thing.
> > That is:
> > two bexti + and and then compare against zero which is exactly the
> > same # of instructions as the above case.

We can form any 2-bit constant with BSETI + BSETI (no OR required).
So no explicit support for that case will be required (as a AND + BEQ
will be formed anyway).

> >
> >
> > >
> > > gcc/ChangeLog:
> > >
> > >         * config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
> > >         Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
> > >         of these tow bits set.
> > >         * config/riscv/predicates.md (const_twobits_operand): New predicate.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * gcc.target/riscv/zbs-if_then_else-01.c: New test.
> > >
> > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > > ---
> > >
> > >  gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
> > >  gcc/config/riscv/predicates.md                |  5 +++
> > >  .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
> > >  3 files changed, 67 insertions(+)
> > >  create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > >
> > > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > > index 7a8f4e35880..2cea394671f 100644
> > > --- a/gcc/config/riscv/bitmanip.md
> > > +++ b/gcc/config/riscv/bitmanip.md
> > > @@ -690,3 +690,45 @@
> > >    "TARGET_ZBS"
> > >    [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
> > >     (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> > > +
> > > +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> > > +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> > > +  [(set (pc)
> > > +       (if_then_else (match_operator 1 "equality_operator"
> > > +                      [(and:X (match_operand:X 2 "register_operand" "r")
> > > +                              (match_operand:X 3 "const_twobits_operand" "i"))
> > > +                       (match_operand:X 4 "single_bit_mask_operand" "i")])
> > > +        (label_ref (match_operand 0 "" ""))
> > > +        (pc)))
> > > +   (clobber (match_scratch:X 5 "=&r"))
> > > +   (clobber (match_scratch:X 6 "=&r"))]
> > > +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"
>
> Is there a reason why you can't do this at expand time? I think there
> are recent patches floating around which is supposed to help with that
> case and the RISCV backend just needs to plug into that infrastructure
> too.

I may have missed the specific patches you refer to (pointer to the
relevant series appreciated).

However, if we move this to expand-time, then ifcvt.cc will run after
(and may form this case once our support for polarity-reversed bit
tests is merged).
So there is good reason to have this pattern.

> Thanks,
> Andrew Pinski
>
> > > +  "#"
> > > +  "&& reload_completed"
> > > +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> > > +                                     (const_int 1)
> > > +                                     (match_dup 8)))
> > > +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> > > +                                     (const_int 1)
> > > +                                     (match_dup 9)))
> > > +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> > > +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> > > +                          (label_ref (match_dup 0))
> > > +                          (pc)))]
> > > +{
> > > +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> > > +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> > > +
> > > +   /* Make sure that the reference value has one of the bits of the mask set */
> > > +   if ((twobits_mask & singlebit_mask) == 0)
> > > +      FAIL;
> > > +
> > > +   int setbit = ctz_hwi (singlebit_mask);
> > > +   int clearbit = ctz_hwi (twobits_mask & ~singlebit_mask);
> > > +
> > > +   operands[1] = gen_rtx_fmt_ee (GET_CODE (operands[1]) == NE ? EQ : NE,
> > > +                                <X:MODE>mode, operands[6], GEN_INT(0));
> > > +
> > > +   operands[8] = GEN_INT (setbit);
> > > +   operands[9] = GEN_INT (clearbit);
> > > +})
> > > diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
> > > index 490bff688a7..6e34829a59b 100644
> > > --- a/gcc/config/riscv/predicates.md
> > > +++ b/gcc/config/riscv/predicates.md
> > > @@ -321,6 +321,11 @@
> > >    (and (match_code "const_int")
> > >         (match_test "popcount_hwi (~UINTVAL (op)) == 2")))
> > >
> > > +;; A CONST_INT operand that has exactly two bits set.
> > > +(define_predicate "const_twobits_operand"
> > > +  (and (match_code "const_int")
> > > +       (match_test "popcount_hwi (UINTVAL (op)) == 2")))
> > > +
> > >  ;; A CONST_INT operand that fits into the unsigned half of a
> > >  ;; signed-immediate after the top bit has been cleared.
> > >  (define_predicate "uimm_extra_bit_operand"
> > > diff --git a/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > > new file mode 100644
> > > index 00000000000..d249a841ff9
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > > @@ -0,0 +1,20 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-march=rv64gc_zbb_zbs -mabi=lp64" } */
> > > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
> >
> > It would be useful to add a rv32 testcase too.
> >
> > Thanks,
> > Andrew Pinski
> >
> > > +
> > > +void g();
> > > +
> > > +void f1 (long a)
> > > +{
> > > +  if ((a & ((1ul << 33) | (1 << 4))) == (1ul << 33))
> > > +    g();
> > > +}
> > > +
> > > +void f2 (long a)
> > > +{
> > > +  if ((a & 0x12) == 0x10)
> > > +    g();
> > > +}
> > > +
> > > +/* { dg-final { scan-assembler-times "bexti\t" 2 } } */
> > > +/* { dg-final { scan-assembler-times "andn\t" 1 } } */
> > > --
> > > 2.34.1
> > >

On Thu, 17 Nov 2022 at 19:56, Andrew Waterman <andrew@sifive.com> wrote:
>
> On Thu, Nov 17, 2022 at 10:52 AM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > On Thu, 17 Nov 2022 at 19:33, Andrew Waterman <andrew@sifive.com> wrote:
> > >
> > > Am I wrong to worry that this will increase dynamic instruction count
> > > when used in a loop?  The obvious code is more efficient when the
> > > constant loads can be hoisted out of a loop.  Or does the cost model
> > > account for this somehow?
> >
> > With this change merged, GCC still hoists the constants out of the
> > loop (just checked with a quick test case).
> > So the cost model seems correct (whether intentionally or accidentally).
>
> Cool, thanks for checking.

We have an updated cost-model for IF_THEN_ELSE brewing, but it didn't
make the cut (and will need some more adjustments and a lot more
testing).
It seems to make a difference on some SPEC workloads.  I don't have a
timeline on finalizing that cost-model improvement yet.

>
> >
> > Thanks,
> > Philipp.
> >
> > >
> > >
> > > On Sun, Nov 13, 2022 at 12:50 PM Philipp Tomsich
> > > <philipp.tomsich@vrull.eu> wrote:
> > > >
> > > > Use Zbs when generating a sequence for "if ((a & twobits) == singlebit) ..."
> > > > that can be expressed as bexti + bexti + andn.
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > >         * config/riscv/bitmanip.md (*branch<X:mode>_mask_twobits_equals_singlebit):
> > > >         Handle "if ((a & T) == C)" using Zbs, when T has 2 bits set and C has one
> > > >         of these tow bits set.
> > > >         * config/riscv/predicates.md (const_twobits_operand): New predicate.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > >         * gcc.target/riscv/zbs-if_then_else-01.c: New test.
> > > >
> > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > > > ---
> > > >
> > > >  gcc/config/riscv/bitmanip.md                  | 42 +++++++++++++++++++
> > > >  gcc/config/riscv/predicates.md                |  5 +++
> > > >  .../gcc.target/riscv/zbs-if_then_else-01.c    | 20 +++++++++
> > > >  3 files changed, 67 insertions(+)
> > > >  create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > > >
> > > > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > > > index 7a8f4e35880..2cea394671f 100644
> > > > --- a/gcc/config/riscv/bitmanip.md
> > > > +++ b/gcc/config/riscv/bitmanip.md
> > > > @@ -690,3 +690,45 @@
> > > >    "TARGET_ZBS"
> > > >    [(set (match_dup 0) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
> > > >     (set (match_dup 0) (xor:X (match_dup 0) (const_int 1)))])
> > > > +
> > > > +;; IF_THEN_ELSE: test for 2 bits of opposite polarity
> > > > +(define_insn_and_split "*branch<X:mode>_mask_twobits_equals_singlebit"
> > > > +  [(set (pc)
> > > > +       (if_then_else (match_operator 1 "equality_operator"
> > > > +                      [(and:X (match_operand:X 2 "register_operand" "r")
> > > > +                              (match_operand:X 3 "const_twobits_operand" "i"))
> > > > +                       (match_operand:X 4 "single_bit_mask_operand" "i")])
> > > > +        (label_ref (match_operand 0 "" ""))
> > > > +        (pc)))
> > > > +   (clobber (match_scratch:X 5 "=&r"))
> > > > +   (clobber (match_scratch:X 6 "=&r"))]
> > > > +  "TARGET_ZBS && TARGET_ZBB && !SMALL_OPERAND (INTVAL (operands[3]))"
> > > > +  "#"
> > > > +  "&& reload_completed"
> > > > +  [(set (match_dup 5) (zero_extract:X (match_dup 2)
> > > > +                                     (const_int 1)
> > > > +                                     (match_dup 8)))
> > > > +   (set (match_dup 6) (zero_extract:X (match_dup 2)
> > > > +                                     (const_int 1)
> > > > +                                     (match_dup 9)))
> > > > +   (set (match_dup 6) (and:X (not:X (match_dup 6)) (match_dup 5)))
> > > > +   (set (pc) (if_then_else (match_op_dup 1 [(match_dup 6) (const_int 0)])
> > > > +                          (label_ref (match_dup 0))
> > > > +                          (pc)))]
> > > > +{
> > > > +   unsigned HOST_WIDE_INT twobits_mask = UINTVAL (operands[3]);
> > > > +   unsigned HOST_WIDE_INT singlebit_mask = UINTVAL (operands[4]);
> > > > +
> > > > +   /* Make sure that the reference value has one of the bits of the mask set */
> > > > +   if ((twobits_mask & singlebit_mask) == 0)
> > > > +      FAIL;
> > > > +
> > > > +   int setbit = ctz_hwi (singlebit_mask);
> > > > +   int clearbit = ctz_hwi (twobits_mask & ~singlebit_mask);
> > > > +
> > > > +   operands[1] = gen_rtx_fmt_ee (GET_CODE (operands[1]) == NE ? EQ : NE,
> > > > +                                <X:MODE>mode, operands[6], GEN_INT(0));
> > > > +
> > > > +   operands[8] = GEN_INT (setbit);
> > > > +   operands[9] = GEN_INT (clearbit);
> > > > +})
> > > > diff --git a/gcc/config/riscv/predicates.md b/gcc/config/riscv/predicates.md
> > > > index 490bff688a7..6e34829a59b 100644
> > > > --- a/gcc/config/riscv/predicates.md
> > > > +++ b/gcc/config/riscv/predicates.md
> > > > @@ -321,6 +321,11 @@
> > > >    (and (match_code "const_int")
> > > >         (match_test "popcount_hwi (~UINTVAL (op)) == 2")))
> > > >
> > > > +;; A CONST_INT operand that has exactly two bits set.
> > > > +(define_predicate "const_twobits_operand"
> > > > +  (and (match_code "const_int")
> > > > +       (match_test "popcount_hwi (UINTVAL (op)) == 2")))
> > > > +
> > > >  ;; A CONST_INT operand that fits into the unsigned half of a
> > > >  ;; signed-immediate after the top bit has been cleared.
> > > >  (define_predicate "uimm_extra_bit_operand"
> > > > diff --git a/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > > > new file mode 100644
> > > > index 00000000000..d249a841ff9
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.target/riscv/zbs-if_then_else-01.c
> > > > @@ -0,0 +1,20 @@
> > > > +/* { dg-do compile } */
> > > > +/* { dg-options "-march=rv64gc_zbb_zbs -mabi=lp64" } */
> > > > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
> > > > +
> > > > +void g();
> > > > +
> > > > +void f1 (long a)
> > > > +{
> > > > +  if ((a & ((1ul << 33) | (1 << 4))) == (1ul << 33))
> > > > +    g();
> > > > +}
> > > > +
> > > > +void f2 (long a)
> > > > +{
> > > > +  if ((a & 0x12) == 0x10)
> > > > +    g();
> > > > +}
> > > > +
> > > > +/* { dg-final { scan-assembler-times "bexti\t" 2 } } */
> > > > +/* { dg-final { scan-assembler-times "andn\t" 1 } } */
> > > > --
> > > > 2.34.1
> > > >