RE: [PATCH] Hexagon (target/hexagon) remove unused functions

[Taylor Simpson]

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On
> Behalf Of Philippe Mathieu-Daudé
> Sent: Wednesday, April 28, 2021 11:49 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org; ale@rev.ng; Brian Cain
> <bcain@quicinc.com>
> Subject: Re: [PATCH] Hexagon (target/hexagon) remove unused functions
> 
> Hi Taylor,
> 
> On 4/29/21 5:32 AM, Taylor Simpson wrote:
> > Remove gen_read_reg and gen_set_byte
> >
> > Reported-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> > ---
> 
> To help git-tools (and reviewers), please use the 'Based-on' tag
> the next time you send a patch depending on another one:
> Based-on: <1617930474-31979-1-git-send-email-tsimpson@quicinc.com>
> 
> >  target/hexagon/genptr.c | 11 -----------
> >  1 file changed, 11 deletions(-)
> >
> > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
> > index 55c7cd8..f93f895 100644
> > --- a/target/hexagon/genptr.c
> > +++ b/target/hexagon/genptr.c
> > @@ -28,12 +28,6 @@
> >  #undef QEMU_GENERATE
> >  #include "gen_tcg.h"
> >
> > -static inline TCGv gen_read_reg(TCGv result, int num)
> > -{
> > -    tcg_gen_mov_tl(result, hex_gpr[num]);
> > -    return result;
> > -}
> 
> But what about:
> 
> target/hexagon/macros.h:26:#define READ_REG(dest, NUM)
> gen_read_reg(dest, NUM)

I should remove this to avoid confusion.

> target/hexagon/macros.h:29:#define READ_REG(NUM)
> (env->gpr[(NUM)])
> target/hexagon/macros.h:360:#define fREAD_LR()
> (READ_REG(HEX_REG_LR))
> target/hexagon/macros.h:366:#define fREAD_SP()
> (READ_REG(HEX_REG_SP))
> target/hexagon/macros.h:367:#define fREAD_LC0
> (READ_REG(HEX_REG_LC0))
> target/hexagon/macros.h:368:#define fREAD_LC1
> (READ_REG(HEX_REG_LC1))
> target/hexagon/macros.h:369:#define fREAD_SA0
> (READ_REG(HEX_REG_SA0))
> target/hexagon/macros.h:370:#define fREAD_SA1
> (READ_REG(HEX_REG_SA1))
> target/hexagon/macros.h:371:#define fREAD_FP()
> (READ_REG(HEX_REG_FP))
> target/hexagon/macros.h:375:    (insn->extension_valid ? 0 :
> READ_REG(HEX_REG_GP))
> target/hexagon/macros.h:377:#define fREAD_GP()
> READ_REG(HEX_REG_GP)
> target/hexagon/macros.h:379:#define fREAD_PC()
> (READ_REG(HEX_REG_PC))
> target/hexagon/macros.h:577:#define fGET_FRAMEKEY()
> READ_REG(HEX_REG_FRAMEKEY)

These are not guarded by QEMU_GENERATE, so they are needed and reference the 
other version of READ_REG

> and:
> 
> target/hexagon/genptr.c:37:static inline TCGv gen_read_preg(TCGv pred,
> uint8_t num)
> target/hexagon/macros.h:27:#define READ_PREG(dest, NUM)
> gen_read_preg(dest, (NUM))

This is needed.  It reads a predicate register.  The gen_read_reg functions 
reads a general purpose register.

> I'd rather send a "!fixup Hexagon (target/hexagon) circular addressing"
> patch (so Richard squashes it there) with:

Richard said he could cherry pick a single patch.
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg05968.html

> 
> -- >8 --
> diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
> index b726c3b7917..bf0e5ae92bb 100644
> --- a/target/hexagon/macros.h
> +++ b/target/hexagon/macros.h
> @@ -22,16 +22,11 @@
>  #include "hex_regs.h"
>  #include "reg_fields.h"
> 
> -#ifdef QEMU_GENERATE
> -#define READ_REG(dest, NUM)              gen_read_reg(dest, NUM)
> -#define READ_PREG(dest, NUM)             gen_read_preg(dest, (NUM))
> -#else
>  #define READ_REG(NUM)                    (env->gpr[(NUM)])
>  #define READ_PREG(NUM)                   (env->pred[NUM])
> 
>  #define WRITE_RREG(NUM, VAL)             log_reg_write(env, NUM, VAL, slot)
>  #define WRITE_PREG(NUM, VAL)             log_pred_write(env, NUM, VAL)
> -#endif
> 
>  #define PCALIGN 4
>  #define PCALIGN_MASK (PCALIGN - 1)
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
> index 55c7cd86a4e..42f25f6f462 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -28,18 +28,6 @@
>  #undef QEMU_GENERATE
>  #include "gen_tcg.h"
> 
> -static inline TCGv gen_read_reg(TCGv result, int num)
> -{
> -    tcg_gen_mov_tl(result, hex_gpr[num]);
> -    return result;
> -}
> -
> -static inline TCGv gen_read_preg(TCGv pred, uint8_t num)
> -{
> -    tcg_gen_mov_tl(pred, hex_pred[num]);
> -    return pred;
> -}
> -
>  static inline void gen_log_predicated_reg_write(int rnum, TCGv val, int
> slot)
>  {
>      TCGv zero = tcg_const_tl(0);
> @@ -319,11 +307,6 @@ static inline void gen_set_half_i64(int N, TCGv_i64
> result, TCGv src)
>      tcg_temp_free_i64(src64);
>  }
> 
> -static inline void gen_set_byte(int N, TCGv result, TCGv src)
> -{
> -    tcg_gen_deposit_tl(result, result, src, N * 8, 8);
> -}
> -
>  static void gen_set_byte_i64(int N, TCGv_i64 result, TCGv src)
>  {
>      TCGv_i64 src64 = tcg_temp_new_i64();
> ---
> 
> NB: untested :)
> 
> Regards,
> 
> Phil.