Re: [RFC PATCH v3 07/34] Hexagon (target/hexagon) scalar core helpers

[Richard Henderson]

On 8/18/20 8:50 AM, Taylor Simpson wrote:
> The majority of helpers are generated.  Define the helper functions needed
> then include the generated file
> 
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
>  target/hexagon/helper.h    |  33 ++++
>  target/hexagon/op_helper.c | 368 
> +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 401 insertions(+)
>  create mode 100644 target/hexagon/helper.h
>  create mode 100644 target/hexagon/op_helper.c
> 
> diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h
> new file mode 100644
> index 0000000..48b1917
> --- /dev/null
> +++ b/target/hexagon/helper.h
> @@ -0,0 +1,33 @@
> +/*
> + *  Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights 
> Reserved.
> + *
> + *  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
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +DEF_HELPER_2(raise_exception, noreturn, env, i32)
> +DEF_HELPER_1(debug_start_packet, void, env)
> +DEF_HELPER_3(debug_check_store_width, void, env, int, int)
> +DEF_HELPER_3(debug_commit_end, void, env, int, int)
> +DEF_HELPER_3(merge_inflight_store1s, s32, env, s32, s32)
> +DEF_HELPER_3(merge_inflight_store1u, s32, env, s32, s32)
> +DEF_HELPER_3(merge_inflight_store2s, s32, env, s32, s32)
> +DEF_HELPER_3(merge_inflight_store2u, s32, env, s32, s32)
> +DEF_HELPER_3(merge_inflight_store4s, s32, env, s32, s32)
> +DEF_HELPER_3(merge_inflight_store4u, s32, env, s32, s32)
> +DEF_HELPER_3(merge_inflight_store8u, s64, env, s32, s64)

Please use DEF_HELPER_FLAGS_N when possible.

You should be able to know when

(1) No exceptions are possible, and nothing is touched except the inputs.  In
this case use TCG_CALL_NO_RWG_SE.

(2) No exceptions are possible, and nothing in env is touched for which you
have created a tcg variable in hexagon_translate_init().  In this case use
TCG_CALL_NO_RWG.

(3) Exceptions are possible, and no tcg variables are touched on the
non-exceptional path.  In this case use TCG_CALL_NO_WG.

> +static inline void log_pred_write(CPUHexagonState *env, int pnum,
> +                                  target_ulong val)

You might be better off letting the compiler decide about inlining.

> +/*
> + * Handle mem_noshuf
> + *
> + * This occurs when there is a load that might need data forwarded
> + * from an inflight store in slot 1.  Note that the load and store
> + * might have different sizes, so we can't simply compare the
> + * addresses.  We merge only the bytes that overlap (if any).
> + */
> +static int64_t merge_bytes(CPUHexagonState *env, target_ulong load_addr,
> +                           int64_t load_data, uint32_t load_width)
> +{
> +    /* Don't do anything if slot 1 was cancelled */
> +    const int store_slot = 1;
> +    if (env->slot_cancelled & (1 << store_slot)) {
> +        return load_data;
> +    }
> +
> +    int store_width = env->mem_log_stores[store_slot].width;
> +    target_ulong store_addr = env->mem_log_stores[store_slot].va;
> +    union {
> +        uint8_t bytes[8];
> +        uint32_t data32;
> +        uint64_t data64;
> +    } retdata, storedata;

Red flag here.  This is assuming that the host and the target are both
little-endian.

> +    int bigmask = ((-load_width) & (-store_width));
> +    if ((load_addr & bigmask) != (store_addr & bigmask)) {

Huh?  This doesn't detect overlap.  Counter example:

  load_addr = 0, load_width = 4,
  store_addr = 1, store_width = 1.

  bigmask = -4 & -1 -> -4.

  (0 & -4) != (1 & -4) -> 0 != 1


> +    while ((i < load_width) && (j < store_width)) {
> +        retdata.bytes[i] = storedata.bytes[j];
> +        i++;
> +        j++;
> +    }
> +    return retdata.data64;

This most definitely requires host-endian adjustment.

> +/* Helpful for printing intermediate values within instructions */
> +void HELPER(debug_value)(CPUHexagonState *env, int32_t value)
> +{
> +    HEX_DEBUG_LOG("value = 0x%x\n", value);
> +}
> +
> +void HELPER(debug_value_i64)(CPUHexagonState *env, int64_t value)
> +{
> +    HEX_DEBUG_LOG("value_i64 = 0x%lx\n", value);
> +}

I think we need to lose all of this.
There are other ways to debug TCG.


r~