[Bug regression/106888] New: [RISCV] Excess andi instructions are generated in gcc.dg/pr90838.c

[Bug regression/106888] New: [RISCV] Excess andi instructions are generated in gcc.dg/pr90838.c

[shihua at iscas dot ac.cn]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

            Bug ID: 106888
           Summary: [RISCV] Excess andi instructions are generated  in
                    gcc.dg/pr90838.c
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: regression
          Assignee: unassigned at gcc dot gnu.org
          Reporter: shihua at iscas dot ac.cn
  Target Milestone: ---

After this patch（
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=c23a9c87cc62bd177fd0d4db6ad34b34e1b9a31f），gcc.dg/pr90838.c
failed.

Before：


        .attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0_zbb1p0"
        .attribute unaligned_access, 0
        .attribute stack_align, 16
        .text

        .align  1
        .globl  ctz2
        .type   ctz2, @function
ctz2:
        ctzw    a0,a0
        ret
        .size   ctz2, .-ctz2
        .align  1
        .globl  ctz3
        .type   ctz3, @function
ctz3:
        ctzw    a0,a0
        ret
        .size   ctz3, .-ctz3


After:

        .attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0_zbb1p0"
        .attribute unaligned_access, 0
        .attribute stack_align, 16
        .text
        .align  1
        .globl  ctz1
        .type   ctz1, @function
ctz2:
        ctzw    a0,a0
        andi    a0,a0,127
        ret
        .size   ctz2, .-ctz2
        .align  1
        .globl  ctz3
        .type   ctz3, @function
ctz3:
        ctzw    a0,a0
        andi    a0,a0,127
        ret
        .size   ctz3, .-ctz3

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[shihua at iscas dot ac.cn]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #1 from shihua at iscas dot ac.cn <shihua at iscas dot ac.cn> ---
This patch is 
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=c23a9c87cc62bd177fd0d4db6ad34b34e1b9a31f

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vineetg at gcc dot gnu.org

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 109574 has been marked as a duplicate of this bug. ***

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #3 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
Debugging of ctz3 case

The insns of interest are:

insn_cost 4 for     6: r74:SI=ctz(r73:DI#0)
      REG_DEAD r73:DI
insn_cost 4 for     7: r72:DI=sign_extend(r74:SI)
      REG_DEAD r74:SI

Before the commit in question, combine is able to mush them

allowing combination of insns 6 and 7
original costs 4 + 4 = 8
replacement cost 8
deferring deletion of insn with uid = 6.
modifying insn i3     7: r72:DI=sign_extend(ctz(r76:DI#0))

With the commit in questions, it takes the new code patch introduced

combine_simplify_rtx

   simplify_context::simplify_unary_operation_1
      case SIGN_EXTEND
+         if (val_signbit_known_clear_p
+             simplify_gen_unary (ZERO_EXTEND, mode, op, GET_MODE (op));

   return expand_compound_operation (x);  // x is ZERO_EXTEND now

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #4 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Vineet, we've got some bits here you might want to play with.  I'm about to
leave for the evening, but I'll put you in touch with Raphael tomorrow
afternoon.

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[roger at nextmovesoftware dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #5 from Roger Sayle <roger at nextmovesoftware dot com> ---
Created attachment 54905
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54905&action=edit
proposed patch

This patch should fix this problem, by adding another pattern the machine
description to also recognize zero_extend of clz/ctz/pcnt, matching the current
pattern that only matches sign_extend.  Clearly for SI operands, the result
must always be 0..32, so sign extension and zero extension are equivalent, and
the zero extension is perhaps (now) the preferred canonical form.

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #6 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Comment on attachment 54905
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54905
proposed patch

So that's a subset of what we've done.  We initially thought that was going to
be enough to solve this class of problems.   But it's actually deeper than just
having a zero_extension variant of this pattern. 

I'll officially submit the zero_extension pattern and the match.pd bits.  The
other pattern we wrote is fugly and I'd like to look at it one more time.

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #7 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
(In reply to Roger Sayle from comment #5)
> Created attachment 54905 [details]
> proposed patch
> 
> This patch should fix this problem, by adding another pattern the machine
> description to also recognize zero_extend of clz/ctz/pcnt, matching the
> current pattern that only matches sign_extend.  Clearly for SI operands, the
> result must always be 0..32, so sign extension and zero extension are
> equivalent, and the zero extension is perhaps (now) the preferred canonical
> form.

Thx for the patch Roger, but as Jeff noted, it alone is not enough and
generates same extra ANDI. Would you have expected combine to recog() the new
pattern ?

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #8 from Jeffrey A. Law <law at gcc dot gnu.org> ---
So coming back to this after a couple months, I'm confident the match.pd change
is unnecessary and in fact wrong.  So we definitely want to set that aside.

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #9 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
(In reply to Jeffrey A. Law from comment #6)
> Comment on attachment 54905 [details]
> proposed patch
> 
> So that's a subset of what we've done.  We initially thought that was going
> to be enough to solve this class of problems.   But it's actually deeper
> than just having a zero_extension variant of this pattern. 

Yeah it seems adding a new define_insn with zero_extend is not enough (nor is
the more elegant any_extend to existing "*<bitmanip_optab>disi2")

Thing is at expand time, we have gimple CTZ expand to ctz+sign_extend, so
adding zero_extend won't really help ?

(insn 6 3 7 2 (set (reg:SI 74)
        (ctz:SI (subreg/s/u:SI (reg/v:DI 73 [ x ]) 0))) "pr90838-red.c":11:15
-1
     (nil))
(insn 7 6 8 2 (set (reg:DI 72 [ <retval> ])
        (sign_extend:DI (reg:SI 74))) "pr90838-red.c":11:15 -1
     (nil))


> I'll officially submit the zero_extension pattern and the match.pd bits. 
> The other pattern we wrote is fugly and I'd like to look at it one more time.

But that other pattern is needed for combine to fuse them together.

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #10 from Jeffrey A. Law <law at gcc dot gnu.org> ---
The sign_extend later gets turned into zero_extend.  Presumably because we know
the value is never negative.  That in and of itself wouldn't be a big deal as
it should be easily recognizable using any_extend.  But combine steps in and
scrambles the RTL in various unhelpful ways.

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:9000da00dd70988f30d43806bae33b22ee6b9904

commit r14-1006-g9000da00dd70988f30d43806bae33b22ee6b9904
Author: Raphael Moreira Zinsly <rzinsly@ventanamicro.com>
Date:   Fri May 19 20:54:34 2023 -0600

    RISC-V: Fix CTZ unnecessary sign extension [PR #106888]

    Changes since v1:
            - Remove subreg from operand 1.

    -- >8 --

    We were not able to match the CTZ sign extend pattern on RISC-V
    because it gets optimized to zero extend and/or to ANDI patterns.
    For the ANDI case, combine scrambles the RTL and generates the
    extension by using subregs.

    gcc/ChangeLog:
            PR target/106888
            * config/riscv/bitmanip.md
            (<bitmanip_optab>disi2): Match with any_extend.
            (<bitmanip_optab>disi2_sext): New pattern to match
            with sign extend using an ANDI instruction.

    gcc/testsuite/ChangeLog:
            PR target/106888
            * gcc.target/riscv/pr106888.c: New test.
            * gcc.target/riscv/zbbw.c: Check for ANDI.

[Bug tree-optimization/106888] [RISCV] Negative optimization that excess andi instructions are generated in gcc.dg/pr90838.c

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106888

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #12 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Should be fixed with Raphael's patch on the trunk.