RE: [PATCH 3/3] Hexagon (target/hexagon) Change decision to set pkt_has_store_s[01]

[Taylor Simpson]

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