From: Taylor Simpson <tsimpson@quicinc.com>
To: Richard Henderson <richard.henderson@linaro.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "ale@rev.ng" <ale@rev.ng>, Brian Cain <bcain@quicinc.com>,
"philmd@redhat.com" <philmd@redhat.com>,
"peter.maydell@linaro.org" <peter.maydell@linaro.org>
Subject: RE: [PATCH 02/20] Hexagon HVX (target/hexagon) add Hexagon Vector eXtensions (HVX) to core
Date: Mon, 26 Jul 2021 04:02:25 +0000 [thread overview]
Message-ID: <BYAPR02MB4886FB40A259261C7E10A2EBDEE89@BYAPR02MB4886.namprd02.prod.outlook.com> (raw)
In-Reply-To: <9d5e6225-4ffa-d394-17b9-518c58842186@linaro.org>
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Sunday, July 25, 2021 8:08 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; ale@rev.ng; Brian Cain <bcain@quicinc.com>;
> peter.maydell@linaro.org
> Subject: Re: [PATCH 02/20] Hexagon HVX (target/hexagon) add Hexagon
> Vector eXtensions (HVX) to core
>
> On 7/5/21 1:34 PM, Taylor Simpson wrote:
> > HVX is a set of wide vector instructions. Machine state includes
> > vector registers (VRegs)
> > vector predicate registers (QRegs)
> > temporary registers for intermediate values
> > store buffer (masked stores and scatter/gather)
> >
> > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> > ---
> > target/hexagon/cpu.h | 35 ++++++++++++++++-
> > target/hexagon/hex_arch_types.h | 5 +++
> > target/hexagon/insn.h | 3 ++
> > target/hexagon/internal.h | 3 ++
> > target/hexagon/mmvec/mmvec.h | 83
> +++++++++++++++++++++++++++++++++++++++++
> > target/hexagon/cpu.c | 72
> ++++++++++++++++++++++++++++++++++-
> > 6 files changed, 198 insertions(+), 3 deletions(-)
> > create mode 100644 target/hexagon/mmvec/mmvec.h
> >
> > diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> > 2855dd3..0b377c3 100644
> > --- a/target/hexagon/cpu.h
> > +++ b/target/hexagon/cpu.h
> > @@ -26,6 +26,7 @@ typedef struct CPUHexagonState CPUHexagonState;
> > #include "qemu-common.h"
> > #include "exec/cpu-defs.h"
> > #include "hex_regs.h"
> > +#include "mmvec/mmvec.h"
> >
> > #define NUM_PREGS 4
> > #define TOTAL_PER_THREAD_REGS 64
> > @@ -34,6 +35,7 @@ typedef struct CPUHexagonState CPUHexagonState;
> > #define STORES_MAX 2
> > #define REG_WRITES_MAX 32
> > #define PRED_WRITES_MAX 5 /* 4 insns + endloop */
> > +#define VSTORES_MAX 2
> >
> > #define TYPE_HEXAGON_CPU "hexagon-cpu"
> >
> > @@ -52,6 +54,13 @@ typedef struct {
> > uint64_t data64;
> > } MemLog;
> >
> > +typedef struct {
> > + target_ulong va;
> > + int size;
> > + MMVector mask QEMU_ALIGNED(16);
> > + MMVector data QEMU_ALIGNED(16);
> > +} VStoreLog;
>
> Do you really need a MMVector mask, or should this be a QRegMask?
You are correct. I'll change this.
>
> > - target_ulong gather_issued;
> > + bool gather_issued;
>
> Surely unrelated to adding state.
This was unintentionally included in the patch series for the scalar core. Based on previous feedback, I know it should be a bool. However, this can actually be removed altogether. So, in the next iteration of this series, you'll see it removed.
>
> > + MMVector VRegs[NUM_VREGS] QEMU_ALIGNED(16);
> > + MMVector future_VRegs[NUM_VREGS] QEMU_ALIGNED(16);
> > + MMVector tmp_VRegs[NUM_VREGS] QEMU_ALIGNED(16);
>
> Ok, this is where I'm not keen on how you handle this for integer code, and
> for vector code has got to be past the realm of acceptable.
>
> You have exactly 4 slots in your vliw packet. You cannot possibly use 32
> future or tmp slots. For integers this wastage was at least small, but for
> vectors these waste just shy of 8k.
>
> All you need to do is to be smarter about mapping e.g. 5 to 8 temp slots
> during translation.
OK
>
> > + MMQReg QRegs[NUM_QREGS] QEMU_ALIGNED(16);
> > + MMQReg future_QRegs[NUM_QREGS] QEMU_ALIGNED(16);
>
> Likewise.
>
> > + /* Temporaries used within instructions */
> > + MMVector zero_vector QEMU_ALIGNED(16);
>
> You must be doing something wrong to need zero in memory.
The architecture specifies that if you use a .new in a packet where the vector register isn't defined, it gets zero. So, the generator produces the following for .new references
const intptr_t OsN_off =
test_bit(OsN_num, ctx->vregs_updated)
? offsetof(CPUHexagonState, future_VRegs[OsN_num])
: offsetof(CPUHexagonState, zero_vector);
>
> > +/*
> > + * The HVX register dump takes up a ton of space in the log
> > + * Don't print it unless it is needed */ #define DUMP_HVX 0 #if
> > +DUMP_HVX
> > + qemu_fprintf(f, "Vector Registers = {\n");
> > + for (int i = 0; i < NUM_VREGS; i++) {
> > + print_vreg(f, env, i);
> > + }
> > + for (int i = 0; i < NUM_QREGS; i++) {
> > + print_qreg(f, env, i);
> > + }
> > + qemu_fprintf(f, "}\n");
> > +#endif
>
> Use CPU_DUMP_FPU, controlled by -d fpu.
These aren't FP registers, but OK.
>
> > - cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS;
> > + cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS + NUM_VREGS
> +
> > + NUM_QREGS;
>
> They're not really core regs though are they?
> Surely gdb_register_coprocessor is a better representation.
I'll look into this.
Thanks,
Taylor
next prev parent reply other threads:[~2021-07-26 4:04 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-05 23:34 [PATCH 00/20] Hexagon HVX (target/hexagon) patch series Taylor Simpson
2021-07-05 23:34 ` [PATCH 01/20] Hexagon HVX (target/hexagon) README Taylor Simpson
2021-07-12 8:16 ` Rob Landley
2021-07-12 13:42 ` Brian Cain
2021-07-19 1:10 ` Rob Landley
2021-07-19 13:39 ` Brian Cain
2021-07-19 16:19 ` Sid Manning
2021-07-26 7:57 ` Rob Landley
2021-07-26 8:54 ` Rob Landley
2021-07-26 13:59 ` Taylor Simpson
2021-07-28 8:11 ` Rob Landley
2021-11-25 6:26 ` Rob Landley
2021-07-05 23:34 ` [PATCH 02/20] Hexagon HVX (target/hexagon) add Hexagon Vector eXtensions (HVX) to core Taylor Simpson
2021-07-25 13:08 ` Richard Henderson
2021-07-26 4:02 ` Taylor Simpson [this message]
2021-07-27 17:21 ` Taylor Simpson
2021-07-05 23:34 ` [PATCH 03/20] Hexagon HVX (target/hexagon) register names Taylor Simpson
2021-07-25 13:10 ` Richard Henderson
2021-07-05 23:34 ` [PATCH 04/20] Hexagon HVX (target/hexagon) support in gdbstub Taylor Simpson
2021-07-05 23:34 ` [PATCH 05/20] Hexagon HVX (target/hexagon) instruction attributes Taylor Simpson
2021-07-05 23:34 ` [PATCH 06/20] Hexagon HVX (target/hexagon) macros Taylor Simpson
2021-07-25 13:13 ` Richard Henderson
2021-07-05 23:34 ` [PATCH 07/20] Hexagon HVX (target/hexagon) import macro definitions Taylor Simpson
2021-07-05 23:34 ` [PATCH 08/20] Hexagon HVX (target/hexagon) semantics generator Taylor Simpson
2021-07-05 23:34 ` [PATCH 09/20] Hexagon HVX (target/hexagon) semantics generator - part 2 Taylor Simpson
2021-07-05 23:34 ` [PATCH 10/20] Hexagon HVX (target/hexagon) C preprocessor for decode tree Taylor Simpson
2021-07-25 13:15 ` Richard Henderson
2021-07-05 23:34 ` [PATCH 11/20] Hexagon HVX (target/hexagon) instruction utility functions Taylor Simpson
2021-07-25 13:21 ` Richard Henderson
2021-07-05 23:34 ` [PATCH 12/20] Hexagon HVX (target/hexagon) helper functions Taylor Simpson
2021-07-25 13:22 ` Richard Henderson
2021-07-26 4:02 ` Taylor Simpson
2021-07-05 23:34 ` [PATCH 13/20] Hexagon HVX (target/hexagon) TCG generation Taylor Simpson
2021-07-05 23:34 ` [PATCH 14/20] Hexagon HVX (target/hexagon) import semantics Taylor Simpson
2021-07-05 23:34 ` [PATCH 15/20] Hexagon HVX (target/hexagon) instruction decoding Taylor Simpson
2021-07-05 23:34 ` [PATCH 16/20] Hexagon HVX (target/hexagon) import instruction encodings Taylor Simpson
2021-07-05 23:34 ` [PATCH 17/20] Hexagon HVX (tests/tcg/hexagon) vector_add_int test Taylor Simpson
2021-07-05 23:34 ` [PATCH 18/20] Hexagon HVX (tests/tcg/hexagon) hvx_misc test Taylor Simpson
2021-07-05 23:34 ` [PATCH 19/20] Hexagon HVX (tests/tcg/hexagon) scatter_gather test Taylor Simpson
2021-07-05 23:34 ` [PATCH 20/20] Hexagon HVX (tests/tcg/hexagon) histogram test Taylor Simpson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BYAPR02MB4886FB40A259261C7E10A2EBDEE89@BYAPR02MB4886.namprd02.prod.outlook.com \
--to=tsimpson@quicinc.com \
--cc=ale@rev.ng \
--cc=bcain@quicinc.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).