Re: [PATCH v3] LoongArch: Replace -mexplicit-relocs=auto simple-used address peephole2 with combine

[chenglulu <chenglulu@loongson.cn>]

在 2023/12/29 上午12:11, Xi Ruoyao 写道:
> The problem with peephole2 is it uses a naive sliding-window algorithm
> and misses many cases.  For example:
>
>      float a[10000];
>      float t() { return a[0] + a[8000]; }
>
> is compiled to:
>
>      la.local    $r13,a
>      la.local    $r12,a+32768
>      fld.s       $f1,$r13,0
>      fld.s       $f0,$r12,-768
>      fadd.s      $f0,$f1,$f0
>
> by trunk.  But as we've explained in r14-4851, the following would be
> better with -mexplicit-relocs=auto:
>
>      pcalau12i   $r13,%pc_hi20(a)
>      pcalau12i   $r12,%pc_hi20(a+32000)
>      fld.s       $f1,$r13,%pc_lo12(a)
>      fld.s       $f0,$r12,%pc_lo12(a+32000)
>      fadd.s      $f0,$f1,$f0
>
> However the sliding-window algorithm just won't detect the pcalau12i/fld
> pair to be optimized.  Use a define_insn_and_split in combine pass will
> work around the issue.
>
> gcc/ChangeLog:
>
> 	* config/loongarch/predicates.md
> 	(symbolic_pcrel_offset_operand): New define_predicate.
> 	(mem_simple_ldst_operand): Likewise.
> 	* config/loongarch/loongarch-protos.h
> 	(loongarch_rewrite_mem_for_simple_ldst): Declare.
> 	* config/loongarch/loongarch.cc
> 	(loongarch_rewrite_mem_for_simple_ldst): Implement.
> 	* config/loongarch/loongarch.md (simple_load<mode>): New
> 	define_insn_and_rewrite.
> 	(simple_load_<su>ext<SUBDI:mode><GPR:mode>): Likewise.
> 	(simple_store<mode>): Likewise.
> 	(define_peephole2): Remove la.local/[f]ld peepholes.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/loongarch/explicit-relocs-auto-single-load-store-2.c:
> 	New test.
> 	* gcc.target/loongarch/explicit-relocs-auto-single-load-store-3.c:
> 	New test.
> ---
>
> Changes from [v2]:
> - Match (mem (symbol_ref ...)) instead of (symbol_ref ...) to retain the
>    attributes of the MEM.
> - Add a test to make sure the attributes of the MEM is retained.
>
> [v2]:https://gcc.gnu.org/pipermail/gcc-patches/2023-December/641430.html
>
> Bootstrapped & regtestd on loongarch64-linux-gnu.  Ok for trunk?
>
>   gcc/config/loongarch/loongarch-protos.h       |   1 +
>   gcc/config/loongarch/loongarch.cc             |  16 +++
>   gcc/config/loongarch/loongarch.md             | 114 +++++-------------
>   gcc/config/loongarch/predicates.md            |  13 ++
>   ...explicit-relocs-auto-single-load-store-2.c |  11 ++
>   ...explicit-relocs-auto-single-load-store-3.c |  18 +++
>   6 files changed, 86 insertions(+), 87 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.target/loongarch/explicit-relocs-auto-single-load-store-2.c
>   create mode 100644 gcc/testsuite/gcc.target/loongarch/explicit-relocs-auto-single-load-store-3.c
>
> diff --git a/gcc/config/loongarch/loongarch-protos.h b/gcc/config/loongarch/loongarch-protos.h

/* snip */

>   
> diff --git a/gcc/config/loongarch/loongarch.md b/gcc/config/loongarch/loongarch.md
/* snip */
> +(define_insn_and_rewrite "simple_load<mode>"
> +  [(set (match_operand:LD_AT_LEAST_32_BIT 0 "register_operand" "=r,f")
> +	(match_operand:LD_AT_LEAST_32_BIT 1 "mem_simple_ldst_operand" ""))]
> +  "loongarch_pre_reload_split () \
> +   && la_opt_explicit_relocs == EXPLICIT_RELOCS_AUTO \
Is the '\' here dispensable? I don't seem to have added it when I wrote 
the conditions.
> +   && (TARGET_CMODEL_NORMAL || TARGET_CMODEL_MEDIUM)"
> +  "#"
> +  "&& true"
>     {
> -    emit_insn (gen_pcalau12i_gr<P:mode> (operands[0], operands[1]));
> +    operands[1] = loongarch_rewrite_mem_for_simple_ldst (operands[1]);
>     })
/* snip */
>   ;; Synchronization instructions.
> diff --git a/gcc/config/loongarch/predicates.md b/gcc/config/loongarch/predicates.md
> index 83fea08315c..2158fe7538c 100644
> --- a/gcc/config/loongarch/predicates.md
> +++ b/gcc/config/loongarch/predicates.md
> @@ -579,6 +579,19 @@ (define_predicate "symbolic_pcrel_operand"
>     return loongarch_symbolic_constant_p (op, &type) && type == SYMBOL_PCREL;
>   })
>   
> +(define_predicate "symbolic_pcrel_offset_operand"
> +  (and (match_code "plus")
> +       (match_operand 0 "symbolic_pcrel_operand")
> +       (match_operand 1 "const_int_operand")))
> +
> +(define_predicate "mem_simple_ldst_operand"
> +  (match_code "mem")
> +{
> +  op = XEXP (op, 0);
> +  return symbolic_pcrel_operand (op, Pmode) ||
> +	 symbolic_pcrel_offset_operand (op, Pmode);
> +})
> +
>   
Symbol '||' It shouldn't be at the end of the line.

+  return symbolic_pcrel_operand (op, Pmode)
+        || symbolic_pcrel_offset_operand (op, Pmode);

Others LGTM.
Thanks!

/* snip */