diff mbox series

RISC-V: Remove the redundant expressions in the and<mode>3.

Message ID 20230714020205.16214-1-lidie@eswincomputing.com
State Unresolved
Headers
Series RISC-V: Remove the redundant expressions in the and<mode>3. |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Die Li July 14, 2023, 2:02 a.m. UTC 
  When generating the gen_and<mode>3 function based on the and<mode>3
template, it produces the expression emit_insn (gen_rtx_SET (operand0,
gen_rtx_AND (<mode>, operand1, operand2)));, which is identical to the
portion I removed in this patch. Therefore, the redundant portion can be
deleted.

Signed-off-by: Die Li <lidie@eswincomputing.com>

gcc/ChangeLog:

        * config/riscv/riscv.md: Remove redundant portion in and<mode>3.
---
 gcc/config/riscv/riscv.md | 5 -----
 1 file changed, 5 deletions(-)

Comments

Palmer Dabbelt July 14, 2023, 2:34 a.m. UTC | #1 
On Thu, 13 Jul 2023 19:02:05 PDT (-0700), lidie@eswincomputing.com wrote:
> When generating the gen_and<mode>3 function based on the and<mode>3
> template, it produces the expression emit_insn (gen_rtx_SET (operand0,
> gen_rtx_AND (<mode>, operand1, operand2)));, which is identical to the
> portion I removed in this patch. Therefore, the redundant portion can be
> deleted.
>
> Signed-off-by: Die Li <lidie@eswincomputing.com>
>
> gcc/ChangeLog:
>
>         * config/riscv/riscv.md: Remove redundant portion in and<mode>3.
> ---
>  gcc/config/riscv/riscv.md | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index 7988026d129..c4f8eb9488e 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -1491,11 +1491,6 @@
>  	  DONE;
>  	}
>      }
> -  else
> -    {
> -      emit_move_insn (operands[0], gen_rtx_AND (<MODE>mode, operands[1], operands[2]));
> -      DONE;
> -    }
>  })
>
>  (define_insn "*and<mode>3"

Unless I'm missing something, this will just result in no emitted 
instructions for this "and" pattern?  That seems wrong, it would at 
least have to put the source into the dest -- but 
"arith_operand_or_mode_mask" can contain values that don't just result 
in an extension (like arbitrary register values, for example), so I 
think we need the "and" operation.

Does this pass the regression suite?

Either way, if this branch of the conditional can't trigger we should 
tighten the constraint (or at a bare minimum add a comment as to why).
Kito Cheng July 14, 2023, 2:41 a.m. UTC | #2 
Expanding without DONE or FAIL will leave the pattern as well, so this
patch is fine IMO, so this patch LGTM, but anyway I will test this and
commit if passed :)

On Fri, Jul 14, 2023 at 10:34 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Thu, 13 Jul 2023 19:02:05 PDT (-0700), lidie@eswincomputing.com wrote:
> > When generating the gen_and<mode>3 function based on the and<mode>3
> > template, it produces the expression emit_insn (gen_rtx_SET (operand0,
> > gen_rtx_AND (<mode>, operand1, operand2)));, which is identical to the
> > portion I removed in this patch. Therefore, the redundant portion can be
> > deleted.
> >
> > Signed-off-by: Die Li <lidie@eswincomputing.com>
> >
> > gcc/ChangeLog:
> >
> >         * config/riscv/riscv.md: Remove redundant portion in and<mode>3.
> > ---
> >  gcc/config/riscv/riscv.md | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> > index 7988026d129..c4f8eb9488e 100644
> > --- a/gcc/config/riscv/riscv.md
> > +++ b/gcc/config/riscv/riscv.md
> > @@ -1491,11 +1491,6 @@
> >         DONE;
> >       }
> >      }
> > -  else
> > -    {
> > -      emit_move_insn (operands[0], gen_rtx_AND (<MODE>mode, operands[1], operands[2]));
> > -      DONE;
> > -    }
> >  })
> >
> >  (define_insn "*and<mode>3"
>
> Unless I'm missing something, this will just result in no emitted
> instructions for this "and" pattern?  That seems wrong, it would at
> least have to put the source into the dest -- but
> "arith_operand_or_mode_mask" can contain values that don't just result
> in an extension (like arbitrary register values, for example), so I
> think we need the "and" operation.
>
> Does this pass the regression suite?
>
> Either way, if this branch of the conditional can't trigger we should
> tighten the constraint (or at a bare minimum add a comment as to why).
Palmer Dabbelt July 14, 2023, 2:44 a.m. UTC | #3 
On Thu, 13 Jul 2023 19:41:08 PDT (-0700), gcc-patches@gcc.gnu.org wrote:
> Expanding without DONE or FAIL will leave the pattern as well, so this
> patch is fine IMO, so this patch LGTM, but anyway I will test this and
> commit if passed :)

Ah, thanks, I guess I didn't know that.  This is probably fine then, but 
we might have some code floating around we could toss...

> On Fri, Jul 14, 2023 at 10:34 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Thu, 13 Jul 2023 19:02:05 PDT (-0700), lidie@eswincomputing.com wrote:
>> > When generating the gen_and<mode>3 function based on the and<mode>3
>> > template, it produces the expression emit_insn (gen_rtx_SET (operand0,
>> > gen_rtx_AND (<mode>, operand1, operand2)));, which is identical to the
>> > portion I removed in this patch. Therefore, the redundant portion can be
>> > deleted.
>> >
>> > Signed-off-by: Die Li <lidie@eswincomputing.com>
>> >
>> > gcc/ChangeLog:
>> >
>> >         * config/riscv/riscv.md: Remove redundant portion in and<mode>3.
>> > ---
>> >  gcc/config/riscv/riscv.md | 5 -----
>> >  1 file changed, 5 deletions(-)
>> >
>> > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
>> > index 7988026d129..c4f8eb9488e 100644
>> > --- a/gcc/config/riscv/riscv.md
>> > +++ b/gcc/config/riscv/riscv.md
>> > @@ -1491,11 +1491,6 @@
>> >         DONE;
>> >       }
>> >      }
>> > -  else
>> > -    {
>> > -      emit_move_insn (operands[0], gen_rtx_AND (<MODE>mode, operands[1], operands[2]));
>> > -      DONE;
>> > -    }
>> >  })
>> >
>> >  (define_insn "*and<mode>3"
>>
>> Unless I'm missing something, this will just result in no emitted
>> instructions for this "and" pattern?  That seems wrong, it would at
>> least have to put the source into the dest -- but
>> "arith_operand_or_mode_mask" can contain values that don't just result
>> in an extension (like arbitrary register values, for example), so I
>> think we need the "and" operation.
>>
>> Does this pass the regression suite?
>>
>> Either way, if this branch of the conditional can't trigger we should
>> tighten the constraint (or at a bare minimum add a comment as to why).
Jeff Law July 14, 2023, 2:50 a.m. UTC | #4 
On 7/13/23 20:44, Palmer Dabbelt wrote:
> On Thu, 13 Jul 2023 19:41:08 PDT (-0700), gcc-patches@gcc.gnu.org wrote:
>> Expanding without DONE or FAIL will leave the pattern as well, so this
>> patch is fine IMO, so this patch LGTM, but anyway I will test this and
>> commit if passed :)
> 
> Ah, thanks, I guess I didn't know that.  This is probably fine then, but 
> we might have some code floating around we could toss...
Yea, if you fall off the end the original pattern stays in place. 
It's always been that way.

And yes, we may have a bit of redundant code and more importantly 
useless RTL generation.

jeff
Jeff Law July 14, 2023, 2:52 a.m. UTC | #5 
On 7/13/23 20:41, Kito Cheng via Gcc-patches wrote:
> Expanding without DONE or FAIL will leave the pattern as well, so this
> patch is fine IMO, so this patch LGTM, but anyway I will test this and
> commit if passed :)
THanks.  I looked fine to me, but I wasn't going to have the time to 
commit/push it tonight.

jeff
Kito Cheng July 14, 2023, 12:32 p.m. UTC | #6 
Committed :)

Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> 於 2023年7月14日 週五 10:52 寫道：

>
>
> On 7/13/23 20:41, Kito Cheng via Gcc-patches wrote:
> > Expanding without DONE or FAIL will leave the pattern as well, so this
> > patch is fine IMO, so this patch LGTM, but anyway I will test this and
> > commit if passed :)
> THanks.  I looked fine to me, but I wasn't going to have the time to
> commit/push it tonight.
>
> jeff
>
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 7988026d129..c4f8eb9488e 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -1491,11 +1491,6 @@ 
 	  DONE;
 	}
     }
-  else
-    {
-      emit_move_insn (operands[0], gen_rtx_AND (<MODE>mode, operands[1], operands[2]));
-      DONE;
-    }
 })
 
 (define_insn "*and<mode>3"