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

[Taylor Simpson]

> -Original Message-
> From: Richard Henderson 
> Sent: Wednesday, August 26, 2020 8:17 AM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@redhat.com; laur...@vivier.eu; riku.voi...@iki.fi;
> aleksandar.m.m...@gmail.com; a...@rev.ng
> Subject: Re: [RFC PATCH v3 07/34] Hexagon (target/hexagon) scalar core
> helpers
>
> > +DEF_HELPER_3(merge_inflight_store8u, s64, env, s32, s64)
>
> Please use DEF_HELPER_FLAGS_N when possible.

OK

> > +static inline void log_pred_write(CPUHexagonState *env, int pnum,
> > +  target_ulong val)
>
> You might be better off letting the compiler decide about inlining.

Isn't "inline" just a hint to the compiler?

> > +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.

OK

> > +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

OK

> > +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.

OK

> > +/* 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.

OK

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 
> ---
>  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 000..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 .
> + */
> +
> +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~