Re: [PATCH] Hexagon: add PC alignment check and exception

[Richard Henderson]

On 4/26/24 11:15, Matheus Tavares Bernardino wrote:
> The Hexagon Programmer's Reference Manual says that the exception 0x1e
> should be raised upon an unaligned program counter. Let's implement that
> and also add tests for both the most common case as well as packets with
> multiple change-of-flow instructions.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>   target/hexagon/cpu_bits.h                  |  1 +
>   target/hexagon/translate.h                 |  2 ++
>   target/hexagon/genptr.c                    | 21 ++++++++++++++++-----
>   target/hexagon/translate.c                 |  2 +-
>   tests/tcg/hexagon/Makefile.target          | 13 +++++++++++++
>   tests/tcg/hexagon/unaligned_pc.S           | 10 ++++++++++
>   tests/tcg/hexagon/unaligned_pc_multi_cof.S | 13 +++++++++++++
>   7 files changed, 56 insertions(+), 6 deletions(-)
>   create mode 100644 tests/tcg/hexagon/unaligned_pc.S
>   create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
>
> diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
> index 96fef71729..d6900c8bda 100644
> --- a/target/hexagon/cpu_bits.h
> +++ b/target/hexagon/cpu_bits.h
> @@ -23,6 +23,7 @@
>   #define HEX_EXCP_FETCH_NO_UPAGE  0x012
>   #define HEX_EXCP_INVALID_PACKET  0x015
>   #define HEX_EXCP_INVALID_OPCODE  0x015
> +#define HEX_EXCP_PC_NOT_ALIGNED  0x01e
>   #define HEX_EXCP_PRIV_NO_UREAD   0x024
>   #define HEX_EXCP_PRIV_NO_UWRITE  0x025
>   
> diff --git a/target/hexagon/translate.h b/target/hexagon/translate.h
> index 4dd59c6726..daf11eb584 100644
> --- a/target/hexagon/translate.h
> +++ b/target/hexagon/translate.h
> @@ -75,6 +75,8 @@ typedef struct DisasContext {
>       TCGv dczero_addr;
>   } DisasContext;
>   
> +void gen_exception_end_tb(DisasContext *ctx, int excp);
> +
>   static inline void ctx_log_pred_write(DisasContext *ctx, int pnum)
>   {
>       if (!test_bit(pnum, ctx->pregs_written)) {
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
> index dbae6c570a..c96edd9379 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv 
> addr,
>                                     TCGCond cond, TCGv pred)
>   {
>       TCGLabel *pred_false = NULL;
> +    TCGLabel *branch_taken = NULL;
>       if (cond != TCG_COND_ALWAYS) {
>           pred_false = gen_new_label();
>           tcg_gen_brcondi_tl(cond, pred, 0, pred_false);
> @@ -480,12 +481,22 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv 
> addr,
>   
>       if (ctx->pkt->pkt_has_multi_cof) {
>           /* If there are multiple branches in a packet, ignore the second one 
> */
> -        tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
> -                           ctx->branch_taken, tcg_constant_tl(0),
> -                           hex_gpr[HEX_REG_PC], addr);
> +        branch_taken = gen_new_label();
> +        tcg_gen_brcondi_tl(TCG_COND_NE, ctx->branch_taken, 0, branch_taken);
>           tcg_gen_movi_tl(ctx->branch_taken, 1);
> -    } else {
> -        tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
> +    }
> +
> +    TCGLabel *pc_aligned = gen_new_label();
> +    TCGv pc_remainder = tcg_temp_new();
> +    tcg_gen_andi_tl(pc_remainder, addr, PCALIGN_MASK);
> +    tcg_gen_brcondi_tl(TCG_COND_EQ, pc_remainder, 0, pc_aligned);
> +    gen_exception_end_tb(ctx, HEX_EXCP_PC_NOT_ALIGNED);
> +    gen_set_label(pc_aligned);
> +
> +    tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);

I am suspicious that the exception is raised without the pc being assigned.
How does the exception handler see the incorrect value?

Also, this is a perfect place to use the new TCG_COND_TSTEQ condition, eliminating the 
separate andi step and the variable.


r~