Re: [PATCH v4 03/11] Hexagon (target/hexagon) Add overrides for S2_asr_r_r_sat/S2_asl_r_r_sat

[Richard Henderson]

On 11/8/22 15:05, Taylor Simpson wrote:
> +/* Shift left with saturation */
> +static void gen_shl_sat(TCGv dst, TCGv src, TCGv shift_amt)
> +{
> +    TCGv_i64 src64 = tcg_temp_local_new_i64();
> +    TCGv_i64 shift64 = tcg_temp_new_i64();
> +    TCGv_i64 dst64 = tcg_temp_new_i64();
> +    TCGv dst_sar = tcg_temp_new();
> +    TCGv ovf = tcg_temp_new();
> +    TCGv satval = tcg_temp_new();
> +    TCGv min = tcg_constant_tl(0x80000000);
> +    TCGv max = tcg_constant_tl(0x7fffffff);
> +
> +    /*
> +     *     dst64 = (int64_t)src << (int64_t)shift_amt
> +     *     dst = (int32_t)dst64
> +     *     dst_sar = dst >> shift_amt
> +     *     if (dst_sar != src) {
> +     *         usr.OVF = 1
> +     *         dst = src < 0 ? min : max
> +     *     }
> +     */
> +    tcg_gen_ext_i32_i64(src64, src);
> +    tcg_gen_ext_i32_i64(shift64, shift_amt);
> +    tcg_gen_shl_i64(dst64, src64, shift64);
> +
> +    tcg_gen_extrl_i64_i32(dst, dst64);
> +    tcg_gen_sar_tl(dst_sar, dst, shift_amt);

I don't think this is quite right.  In particular:

> +static void gen_asr_r_r_sat(TCGv RdV, TCGv RsV, TCGv RtV)
> +{
> +    TCGv shift_amt = tcg_temp_local_new();
> +    TCGLabel *positive = gen_new_label();
> +    TCGLabel *done = gen_new_label();
> +
> +    tcg_gen_sextract_i32(shift_amt, RtV, 0, 7);

This suggests shift amounts -64 ... 63.

> +    tcg_gen_brcondi_tl(TCG_COND_GE, shift_amt, 0, positive);
> +
> +    /* Negative shift amount => shift left */
> +    tcg_gen_neg_tl(shift_amt, shift_amt);

-64 -> 64.

So!  We have two out-of-range shifts in gen_shl_sat, both i64 and i32.
If we fix one, then we don't even need the extension to i64 either.

Consider

    /*
     * sh32 = shift & 31;
     * dst = sh32 == shift ? src : 0;
     * dst <<= sh32;
     * dst_sar = dst >> sh32;
     * if (dst_sar != src) ...
     */
    tcg_gen_andi_i32(sh32, shift_amt, 31);
    tcg_gen_movcond_i32(TCG_COND_EQ, dst,
                        sh32, shift_amt,
                        src, tcg_constant_i32(0));
    tcg_gen_shl_i32(dst, dst, sh32);
    tcg_gen_sar_i32(dst_sar, dst, sh32);


r~