On Tue, 1 Apr 2025 22:57:33 -0600, Jeff Law wrote:
>
>
> On 4/1/25 12:20 AM, Jin Ma wrote:
> > Assuming we have the following variables:
> >
> > unsigned long long a0, a1;
> > unsigned int a2;
> >
> > For the expression:
> >
> > a0 = (a0 << 50) >> 49; // slli a0, a0, 50 + srli a0, a0, 49
> > a2 = a1 + a0; // addw a2, a1, a0 + slli a2, a2, 32 + srli a2, a2,
> > 32
> >
> > In the optimization process of ZBA (combine pass), it would be optimized to:
> >
> > a2 = a0 << 1 + a1; // sh1add a2, a0, a1 + zext.w a2, a2
> >
> > This is clearly incorrect, as it overlooks the fact that a0=a0&0x7ffe,
> > meaning
> > that the bits a0[32:14] are set to zero.
> >
> > gcc/ChangeLog:
> >
> > * config/riscv/bitmanip.md: The optimization can only be applied if
> > the high bit of operands[3] is set to 1.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/riscv/zba-shNadd-09.c: New test.
> > * gcc.target/riscv/zba-shNadd-10.c: New test.
> > ---
> > gcc/config/riscv/bitmanip.md | 3 ++-
> > .../gcc.target/riscv/zba-shNadd-09.c | 12 +++++++++++
> > .../gcc.target/riscv/zba-shNadd-10.c | 20 +++++++++++++++++++
> > 3 files changed, 34 insertions(+), 1 deletion(-)
> > create mode 100644 gcc/testsuite/gcc.target/riscv/zba-shNadd-09.c
> > create mode 100644 gcc/testsuite/gcc.target/riscv/zba-shNadd-10.c
> >
> > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > index b29c127bcb8..9091c48b106 100644
> > --- a/gcc/config/riscv/bitmanip.md
> > +++ b/gcc/config/riscv/bitmanip.md
> > @@ -80,7 +80,8 @@ (define_split
> > (match_operand:DI 3
> > "consecutive_bits_operand")) 0)
> > (subreg:SI (match_operand:DI 4
> > "register_operand") 0))))]
> > "TARGET_64BIT && TARGET_ZBA
> > - && riscv_shamt_matches_mask_p (INTVAL (operands[2]), INTVAL
> > (operands[3]))"
> > + && riscv_shamt_matches_mask_p (INTVAL (operands[2]), INTVAL
> > (operands[3]))
> > + && ((INTVAL (operands[3]) & (1 << 31)) != 0)"
> > [(set (match_dup 0) (plus:DI (ashift:DI (match_dup 1) (match_dup 2))
> > (match_dup 4)))
> > (set (match_dup 0) (zero_extend:DI (subreg:SI (match_dup 0) 0)))])
> So I would add a comment to that new condition. Something like
> /* Ensure the mask includes all the bits in SImode. */
>
> We need to be careful with constants like 1 << 31. Something like
> (HOST_WIDE_INT_1U << 31) would be better from a type safety standpoint
> (INTVAL returns a HOST_WIDE_INT object).
>
> > +#include <stdio.h>
> In general, avoid #includes if you can in tests. Remove the <stdio.h>
> include.
>
> > + printf("%llu\n", d);
> Instead of a printf and using dg-output, the standard way we test for
> correctness is by either aborting or calling exit (0). So rather than
> the printf use
>
> if (d != 3023282)
> __builtin_abort ();
> __builtin_exit (0);
>
> And drop the dg-output line.
>
>
> With those changes this patch is probably OK. We'd want to post the V2
> so that the pre-commit tester can chew on it.
Yes, Very professional advice. I will make the changes and submit V2.
Best regards,
Jin Ma
