Re: [PATCH v4 12/16] target/riscv: implement zicfiss instructions

[Richard Henderson]

On 8/16/24 11:07, Deepak Gupta wrote:
> +static bool trans_ssamoswap_w(DisasContext *ctx, arg_amoswap_w *a)
> +{
> +    REQUIRE_A_OR_ZAAMO(ctx);
> +    /* back cfi was not enabled, return false */
> +    if (!ctx->bcfi_enabled) {
> +        return false;
> +    }

Please drop all of these comments which exactly mirror the code and provide no additional 
insight.

> +static bool gen_sspopchk(DisasContext *ctx, int rs1_reg)
> +{
> +    /* back cfi was not enabled, return false */
> +    if (!ctx->bcfi_enabled) {
> +        return false;
> +    }
> +
> +    /*
> +     * sspopchk can only compare with x1 or x5. Everything else defaults to
> +     * zimops
> +     */
> +
> +    assert(rs1_reg == 1 || rs1_reg == 5);

These asserts seems redundant.  Yes, this is true and a result of the encoding, but 
nothing in this function would misbehave if some future extension allowed any other 
register number.


r~