RE: [PATCH 21/38] target/hexagon: Add system reg insns

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Friday, February 28, 2025 11:26 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; quic_mathbern@quicinc.com; ale@rev.ng; anjo@rev.ng;
> quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com; Brian Cain <bcain@quicinc.com>
> Subject: [PATCH 21/38] target/hexagon: Add system reg insns
> 
> From: Brian Cain <bcain@quicinc.com>
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/macros.h               |   2 +
>  target/hexagon/hex_common.py          |  15 +-
>  target/hexagon/imported/encode_pp.def | 213 +++++++++++++++------
>  target/hexagon/imported/system.idef   | 262
> +++++++++++++++++++++++---
>  4 files changed, 410 insertions(+), 82 deletions(-)
> 
> diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
> 6e4a3a1697..b0e9610d98 100644
> --- a/target/hexagon/macros.h
> +++ b/target/hexagon/macros.h
> @@ -675,3 +675,5 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv
> val, int shift)  #endif
> 
>  #endif
> +
> +#define fPREDUSE_TIMING()

Why wasn't this in the previous patch with all the changes to macros.h?

> diff --git a/target/hexagon/hex_common.py
> b/target/hexagon/hex_common.py index 7fb11a0819..9147701333 100755
> --- a/target/hexagon/hex_common.py
> +++ b/target/hexagon/hex_common.py
> @@ -1235,11 +1235,18 @@ def init_registers():
>      for reg in new_regs:
>          new_registers[f"{reg.regtype}{reg.regid}"] = reg
> 
> +def is_new_reg(tag, regid):
> +    if regid[0] in "NO":
> +        return True
> +    return regid[0] == "P" and \
> +           f"{regid}N" in semdict[tag] and \
> +           f"{regid}V" not in semdict[tag]
> +
>  def get_register(tag, regtype, regid):
> -    if f"{regtype}{regid}V" in semdict[tag]:
> -        return registers[f"{regtype}{regid}"]
> -    else:
> -        return new_registers[f"{regtype}{regid}"]
> +    regid = f"{regtype}{regid}"
> +    is_new = is_new_reg(tag, regid)
> +    reg = new_registers[regid] if is_new else registers[regid]
> +    return reg

This looks OK but is out of place in this patch.

> 
>  def helper_ret_type(tag, regs):
>      ## If there is a scalar result, it is the return type diff --git
> a/target/hexagon/imported/encode_pp.def
> b/target/hexagon/imported/encode_pp.def
> index 0cd30a5e85..37faf62b1b 100644
> --- a/target/hexagon/imported/encode_pp.def
> +++ b/target/hexagon/imported/encode_pp.def
> @@ -1,5 +1,5 @@
>  /*
> - *  Copyright(c) 2019-2023 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + *  Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights
> Reserved.

Why are you changing the copyright date badkward?

>   *
>   *  This program is free software; you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published by @@
> -16,6 +16,7 @@
>   */
> 
>  /*
> + * encode32.def
>   * Encodings for 32 bit instructions
>   *
>   */
> @@ -341,6 +342,8 @@ DEF_ENC32(L4_pload##TAG##tnew_abs,ICLASS_LD"
> 1 11 "OPC"  iiiii  PP110tti  1--ddd
> DEF_ENC32(L4_pload##TAG##fnew_abs,ICLASS_LD" 1 11 "OPC"  iiiii  PP111tti
> 1--ddddd")
> 
> 
> +
> +
>  /*               0 000  misc: dealloc,loadw_locked,dcfetch      */
>  STD_LD_ENC(bzw4,"0 101")
>  STD_LD_ENC(bzw2,"0 011")
> @@ -375,6 +378,7 @@ DEF_ANTICLASS32(ICLASS_LD" 1110 000----- PP------ --
> ------",LD_ADDR_POST_REG)
> 
>  DEF_ENC32(L2_deallocframe,    ICLASS_LD" 000 0 000 sssss PP0----- ---
> ddddd")
>  DEF_ENC32(L4_return,          ICLASS_LD" 011 0 000 sssss PP0000-- ---ddddd")
> +
>  DEF_ENC32(L4_return_t,        ICLASS_LD" 011 0 000 sssss PP0100vv ---ddddd")
>  DEF_ENC32(L4_return_f,        ICLASS_LD" 011 0 000 sssss PP1100vv ---ddddd")
>  DEF_ENC32(L4_return_tnew_pt,  ICLASS_LD" 011 0 000 sssss PP0110vv ---
> ddddd") @@ -382,15 +386,18 @@ DEF_ENC32(L4_return_fnew_pt,
> ICLASS_LD" 011 0 000 sssss PP1110vv ---ddddd")
> DEF_ENC32(L4_return_tnew_pnt, ICLASS_LD" 011 0 000 sssss PP0010vv ---
> ddddd")  DEF_ENC32(L4_return_fnew_pnt, ICLASS_LD" 011 0 000 sssss
> PP1010vv ---ddddd")
> 
> -DEF_ENC32(L2_loadw_locked,ICLASS_LD" 001 0 000 sssss PP000---
> 000ddddd")
> -
> +/** Load Acquire Store Release Encoding **/
> 
> +DEF_ENC32(L2_loadw_locked,    ICLASS_LD" 001 0 000 sssss PP000---
> 000ddddd")
> +DEF_ENC32(L4_loadd_locked,    ICLASS_LD" 001 0 000 sssss PP010---
> 000ddddd")

There are lots of changes here that look like formatting/whitespace.  It would be easier to review if you could minimize these.

> diff --git a/target/hexagon/imported/system.idef
> b/target/hexagon/imported/system.idef
> index 7c6568e75e..fd7ef18b3e 100644
> --- a/target/hexagon/imported/system.idef
> +++ b/target/hexagon/imported/system.idef
> @@ -25,44 +25,262 @@
>  /* User->OS interface                       */
>  /********************************************/
> 
> -Q6INSN(J2_trap0,"trap0(#u8)",ATTRIBS(A_COF),
> +Q6INSN(J2_trap0,"trap0(#u8)",ATTRIBS(A_COF,A_NOTE_NOPACKET,A_RES
> TRICT_N
> +OPACKET),
>  "Trap to Operating System",
> -    fTRAP(0,uiV);
> +	fTRAP(0,uiV);

The formatting/whitespace changes are not as rampant in this file, but this is one example.

>  )