On Mon, Mar 24, 2025 at 3:44 AM Bohan Lei <garth...@linux.alibaba.com> wrote:
>
> The combine pass can generate an index like (and:DI (mult:DI (reg:DI)
> (const_int scale)) (const_int mask)) when XTheadMemIdx is available.
> LRA may pull it out, and thus a splitter is needed when Zba is not
> available.
>
> A similar splitter were introduced when XTheadMemIdx support was added,
> but removed in commit 31c3c5d. The new splitter in this new patch is
> based on the removed one.
Thanks for reporting!
I have confirmed that the code in the test case (of this patch) triggers an ICE.
However, I don't see how this could get reproduced without the inline
assembly part.
History context:
The splitter was needed because we did not guide the combiner properly
in identifying INSNs that can be lowered. So we just let the combiner
do something
and fix that up later with tons of splitters. In commit 31c3c5d we
cleaned up the code
by properly implementing th_memidx_classify_address_index().
Looking at the test case, we can do a simple modification to get a bit
more insight:
int a;
int **b;
int**
c ()
{
int **e = &b[(unsigned)(long)&a];
__asm__ ("" : "+A"(*e));
return e;
}
WIth the code above, the ICE is prevented because the combiner properly lowers
the address calculation into a zero_extendsidi2_shifted.
Replacing "return e" by "return 0" triggers the ICE, because the combiner gets
//address calculation INSNs
//...
(set (mem:DI (reg:DI)) (asm_operands:DI))
and produces an address index of the form
(and:DI (mult:DI (reg:DI) (const_int scale)) (const_int mask) (reg:DI))
So, something about the inline assembly statement causes the combiner to
produce an insn that can't be lowered later on. Preventing this (in
the combiner)
would be the proper fix for this issue.
That said, the splitter is probably the most trivial solution here.
If maintainers share this opinion, then we should probably adjust
the comment above the splitter. E.g.
Help reload to add a displacement for the base register.
This is needed if the combiner produces INSNs that can't be lowered
because of inline-assembly statements.
BR
Christoph
>
> gcc/ChangeLog:
>
> * config/riscv/thead.md (*th_memidx_operand): New splitter.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/xtheadmemidx-bug.c: New test.
> ---
> gcc/config/riscv/thead.md | 21 +++++++++++++++++++
> .../gcc.target/riscv/xtheadmemidx-bug.c | 13 ++++++++++++
> 2 files changed, 34 insertions(+)
> create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadmemidx-bug.c
>
> diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
> index d816f3b86dd..20e82e68df2 100644
> --- a/gcc/config/riscv/thead.md
> +++ b/gcc/config/riscv/thead.md
> @@ -458,6 +458,27 @@ (define_insn "*th_mempair_load_zero_extendsidi2"
>
> ;; XTheadMemIdx
>
> +;; Help reload to add a displacement for the base register.
> +;; In the case `zext(*(uN*))(base+((rN<<1)&0x1fffffffe))` LRA splits
> +;; off two new instructions: a) `new_base = base + disp`, and
> +;; b) `index = (rN<<1)&0x1fffffffe`. The index calculation has no
> +;; corresponding instruction pattern and needs this insn_and_split
> +;; to recover.
> +
> +(define_insn_and_split "*th_memidx_operand"
> + [(set (match_operand:DI 0 "register_operand" "=r")
> + (and:DI (ashift:DI (match_operand:DI 1 "register_operand" "r")
> + (match_operand:QI 2 "imm123_operand" "Ds3"))
> + (match_operand 3 "const_int_operand" "n")))]
> + "TARGET_64BIT && TARGET_XTHEADMEMIDX && (lra_in_progress ||
> reload_completed)
> + && (INTVAL (operands[3]) >> INTVAL (operands[2])) == 0xffffffff"
> + "#"
> + "&& !TARGET_ZBA && reload_completed"
> + [(set (match_dup 0) (zero_extend:DI (subreg:SI (match_dup 1) 0)))
> + (set (match_dup 0) (ashift:DI (match_dup 0) (match_dup 2)))]
> + ""
> + [(set_attr "type" "bitmanip")])
> +
> (define_insn "*th_memidx_zero_extendqi<SUPERQI:mode>2"
> [(set (match_operand:SUPERQI 0 "register_operand" "=r,r,r,r,r,r")
> (zero_extend:SUPERQI
> diff --git a/gcc/testsuite/gcc.target/riscv/xtheadmemidx-bug.c
> b/gcc/testsuite/gcc.target/riscv/xtheadmemidx-bug.c
> new file mode 100644
> index 00000000000..92b24e311b6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/xtheadmemidx-bug.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile { target { rv64 } } } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" "-Os" "-Og" "-Oz" } } */
> +/* { dg-options "-march=rv64gc_xtheadmemidx" } */
> +
> +int a;
> +int **b;
> +
> +void
> +c ()
> +{
> + int **e = &b[(unsigned)(long)&a];
> + __asm__ ("" : "+A"(*e));
> +}
> --
> 2.46.0
>
