| Message ID | 20220920080746.26791-4-tsimpson@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hexagon (target/hexagon) improve store handling | expand |
On 9/20/22 01:07, Taylor Simpson wrote: > We have found cases where pkt_has_store_s[01] is set incorrectly. > This leads to generating an unnecessary store that is left over > from a previous packet. > > Add an attribute to determine if an instruction is a scalar store > The attribute is attached to the fSTORE macro (hex_common.py) > Simplify the logic in decode.c that sets pkt_has_store_s[01] > > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> > --- > target/hexagon/attribs_def.h.inc | 1 + > target/hexagon/decode.c | 17 ++++++++++++----- > target/hexagon/translate.c | 10 ++++++---- > target/hexagon/hex_common.py | 3 ++- > 4 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/target/hexagon/attribs_def.h.inc b/target/hexagon/attribs_def.h.inc > index 222ad95fb0..5d2a102c18 100644 > --- a/target/hexagon/attribs_def.h.inc > +++ b/target/hexagon/attribs_def.h.inc > @@ -44,6 +44,7 @@ DEF_ATTRIB(MEMSIZE_1B, "Memory width is 1 byte", "", "") > DEF_ATTRIB(MEMSIZE_2B, "Memory width is 2 bytes", "", "") > DEF_ATTRIB(MEMSIZE_4B, "Memory width is 4 bytes", "", "") > DEF_ATTRIB(MEMSIZE_8B, "Memory width is 8 bytes", "", "") > +DEF_ATTRIB(SCALAR_STORE, "Store is scalar", "", "") > DEF_ATTRIB(REGWRSIZE_1B, "Memory width is 1 byte", "", "") > DEF_ATTRIB(REGWRSIZE_2B, "Memory width is 2 bytes", "", "") > DEF_ATTRIB(REGWRSIZE_4B, "Memory width is 4 bytes", "", "") > diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c > index 6f0f27b4ba..2ba94a77de 100644 > --- a/target/hexagon/decode.c > +++ b/target/hexagon/decode.c > @@ -1,5 +1,5 @@ > /* > - * Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. > + * Copyright(c) 2019-2022 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 > @@ -402,10 +402,17 @@ static void decode_set_insn_attr_fields(Packet *pkt) > } > > if (GET_ATTRIB(opcode, A_STORE)) { > - if (pkt->insn[i].slot == 0) { > - pkt->pkt_has_store_s0 = true; > - } else { > - pkt->pkt_has_store_s1 = true; > + if (GET_ATTRIB(opcode, A_SCALAR_STORE) && > + !GET_ATTRIB(opcode, A_MEMSIZE_0B)) { > + g_assert(GET_ATTRIB(opcode, A_MEMSIZE_1B) || > + GET_ATTRIB(opcode, A_MEMSIZE_2B) || > + GET_ATTRIB(opcode, A_MEMSIZE_4B) || > + GET_ATTRIB(opcode, A_MEMSIZE_8B)); Would this assert be redundant with the one I suggested vs patch 2? Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ > + if (pkt->insn[i].slot == 0) { > + pkt->pkt_has_store_s0 = true; > + } else { > + pkt->pkt_has_store_s1 = true; > + } > } > } > > diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c > index bc02870b9f..efe7d2264e 100644 > --- a/target/hexagon/translate.c > +++ b/target/hexagon/translate.c > @@ -1,5 +1,5 @@ > /* > - * Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. > + * Copyright(c) 2019-2022 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 > @@ -525,10 +525,12 @@ static void process_store_log(DisasContext *ctx, Packet *pkt) > * slot 1 and then slot 0. This will be important when > * the memory accesses overlap. > */ > - if (pkt->pkt_has_store_s1 && !pkt->pkt_has_dczeroa) { > + if (pkt->pkt_has_store_s1) { > + g_assert(!pkt->pkt_has_dczeroa); > process_store(ctx, pkt, 1); > } > - if (pkt->pkt_has_store_s0 && !pkt->pkt_has_dczeroa) { > + if (pkt->pkt_has_store_s0) { > + g_assert(!pkt->pkt_has_dczeroa); > process_store(ctx, pkt, 0); > } > } > @@ -691,7 +693,7 @@ static void gen_commit_packet(CPUHexagonState *env, DisasContext *ctx, > * The dczeroa will be the store in slot 0, check that we don't have > * a store in slot 1 or an HVX store. > */ > - g_assert(has_store_s0 && !has_store_s1 && !has_hvx_store); > + g_assert(!has_store_s1 && !has_hvx_store); > process_dczeroa(ctx, pkt); > } else if (has_hvx_store) { > TCGv mem_idx = tcg_constant_tl(ctx->mem_idx); > diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py > index c81aca8d2a..d9ba7df786 100755 > --- a/target/hexagon/hex_common.py > +++ b/target/hexagon/hex_common.py > @@ -1,7 +1,7 @@ > #!/usr/bin/env python3 > > ## > -## Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. > +## Copyright(c) 2019-2022 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 > @@ -75,6 +75,7 @@ def calculate_attribs(): > add_qemu_macro_attrib('fWRITE_P3', 'A_WRITES_PRED_REG') > add_qemu_macro_attrib('fSET_OVERFLOW', 'A_IMPLICIT_WRITES_USR') > add_qemu_macro_attrib('fSET_LPCFG', 'A_IMPLICIT_WRITES_USR') > + add_qemu_macro_attrib('fSTORE', 'A_SCALAR_STORE') > > # Recurse down macros, find attributes from sub-macros > macroValues = list(macros.values())
> -----Original Message----- > From: Richard Henderson <richard.henderson@linaro.org> > Sent: Wednesday, September 28, 2022 11:12 AM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: f4bug@amsat.org; ale@rev.ng; anjo@rev.ng; Brian Cain > <bcain@quicinc.com>; Michael Lambert <mlambert@quicinc.com> > Subject: Re: [PATCH 3/3] Hexagon (target/hexagon) Change decision to set > pkt_has_store_s[01] > > On 9/20/22 01:07, Taylor Simpson wrote: > > We have found cases where pkt_has_store_s[01] is set incorrectly. > > This leads to generating an unnecessary store that is left over from a > > previous packet. > > > > Add an attribute to determine if an instruction is a scalar store The > > attribute is attached to the fSTORE macro (hex_common.py) Simplify the > > logic in decode.c that sets pkt_has_store_s[01] > > > > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> > > --- > > target/hexagon/attribs_def.h.inc | 1 + > > target/hexagon/decode.c | 17 ++++++++++++----- > > target/hexagon/translate.c | 10 ++++++---- > > target/hexagon/hex_common.py | 3 ++- > > 4 files changed, 21 insertions(+), 10 deletions(-) > > > > --git a/target/hexagon/decode.c b/target/hexagon/decode.c index > > 6f0f27b4ba..2ba94a77de 100644 > > --- a/target/hexagon/decode.c > > +++ b/target/hexagon/decode.c > > @@ -1,5 +1,5 @@ > > } > > > > if (GET_ATTRIB(opcode, A_STORE)) { > > - if (pkt->insn[i].slot == 0) { > > - pkt->pkt_has_store_s0 = true; > > - } else { > > - pkt->pkt_has_store_s1 = true; > > + if (GET_ATTRIB(opcode, A_SCALAR_STORE) && > > + !GET_ATTRIB(opcode, A_MEMSIZE_0B)) { > > + g_assert(GET_ATTRIB(opcode, A_MEMSIZE_1B) || > > + GET_ATTRIB(opcode, A_MEMSIZE_2B) || > > + GET_ATTRIB(opcode, A_MEMSIZE_4B) || > > + GET_ATTRIB(opcode, A_MEMSIZE_8B)); > > Would this assert be redundant with the one I suggested vs patch 2? > > Otherwise, > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Yes, this would be redundant with the one you suggested. Further, the one you suggested is an improvement because it ensures that exactly one of the attributes is set. Will make the changes and create a PR. Thanks, Taylor
diff --git a/target/hexagon/attribs_def.h.inc b/target/hexagon/attribs_def.h.inc index 222ad95fb0..5d2a102c18 100644 --- a/target/hexagon/attribs_def.h.inc +++ b/target/hexagon/attribs_def.h.inc @@ -44,6 +44,7 @@ DEF_ATTRIB(MEMSIZE_1B, "Memory width is 1 byte", "", "") DEF_ATTRIB(MEMSIZE_2B, "Memory width is 2 bytes", "", "") DEF_ATTRIB(MEMSIZE_4B, "Memory width is 4 bytes", "", "") DEF_ATTRIB(MEMSIZE_8B, "Memory width is 8 bytes", "", "") +DEF_ATTRIB(SCALAR_STORE, "Store is scalar", "", "") DEF_ATTRIB(REGWRSIZE_1B, "Memory width is 1 byte", "", "") DEF_ATTRIB(REGWRSIZE_2B, "Memory width is 2 bytes", "", "") DEF_ATTRIB(REGWRSIZE_4B, "Memory width is 4 bytes", "", "") diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c index 6f0f27b4ba..2ba94a77de 100644 --- a/target/hexagon/decode.c +++ b/target/hexagon/decode.c @@ -1,5 +1,5 @@ /* - * Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. + * Copyright(c) 2019-2022 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 @@ -402,10 +402,17 @@ static void decode_set_insn_attr_fields(Packet *pkt) } if (GET_ATTRIB(opcode, A_STORE)) { - if (pkt->insn[i].slot == 0) { - pkt->pkt_has_store_s0 = true; - } else { - pkt->pkt_has_store_s1 = true; + if (GET_ATTRIB(opcode, A_SCALAR_STORE) && + !GET_ATTRIB(opcode, A_MEMSIZE_0B)) { + g_assert(GET_ATTRIB(opcode, A_MEMSIZE_1B) || + GET_ATTRIB(opcode, A_MEMSIZE_2B) || + GET_ATTRIB(opcode, A_MEMSIZE_4B) || + GET_ATTRIB(opcode, A_MEMSIZE_8B)); + if (pkt->insn[i].slot == 0) { + pkt->pkt_has_store_s0 = true; + } else { + pkt->pkt_has_store_s1 = true; + } } } diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c index bc02870b9f..efe7d2264e 100644 --- a/target/hexagon/translate.c +++ b/target/hexagon/translate.c @@ -1,5 +1,5 @@ /* - * Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. + * Copyright(c) 2019-2022 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 @@ -525,10 +525,12 @@ static void process_store_log(DisasContext *ctx, Packet *pkt) * slot 1 and then slot 0. This will be important when * the memory accesses overlap. */ - if (pkt->pkt_has_store_s1 && !pkt->pkt_has_dczeroa) { + if (pkt->pkt_has_store_s1) { + g_assert(!pkt->pkt_has_dczeroa); process_store(ctx, pkt, 1); } - if (pkt->pkt_has_store_s0 && !pkt->pkt_has_dczeroa) { + if (pkt->pkt_has_store_s0) { + g_assert(!pkt->pkt_has_dczeroa); process_store(ctx, pkt, 0); } } @@ -691,7 +693,7 @@ static void gen_commit_packet(CPUHexagonState *env, DisasContext *ctx, * The dczeroa will be the store in slot 0, check that we don't have * a store in slot 1 or an HVX store. */ - g_assert(has_store_s0 && !has_store_s1 && !has_hvx_store); + g_assert(!has_store_s1 && !has_hvx_store); process_dczeroa(ctx, pkt); } else if (has_hvx_store) { TCGv mem_idx = tcg_constant_tl(ctx->mem_idx); diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py index c81aca8d2a..d9ba7df786 100755 --- a/target/hexagon/hex_common.py +++ b/target/hexagon/hex_common.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 ## -## Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. +## Copyright(c) 2019-2022 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 @@ -75,6 +75,7 @@ def calculate_attribs(): add_qemu_macro_attrib('fWRITE_P3', 'A_WRITES_PRED_REG') add_qemu_macro_attrib('fSET_OVERFLOW', 'A_IMPLICIT_WRITES_USR') add_qemu_macro_attrib('fSET_LPCFG', 'A_IMPLICIT_WRITES_USR') + add_qemu_macro_attrib('fSTORE', 'A_SCALAR_STORE') # Recurse down macros, find attributes from sub-macros macroValues = list(macros.values())
We have found cases where pkt_has_store_s[01] is set incorrectly. This leads to generating an unnecessary store that is left over from a previous packet. Add an attribute to determine if an instruction is a scalar store The attribute is attached to the fSTORE macro (hex_common.py) Simplify the logic in decode.c that sets pkt_has_store_s[01] Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> --- target/hexagon/attribs_def.h.inc | 1 + target/hexagon/decode.c | 17 ++++++++++++----- target/hexagon/translate.c | 10 ++++++---- target/hexagon/hex_common.py | 3 ++- 4 files changed, 21 insertions(+), 10 deletions(-)