> -----Original Message----- > From: Richard Henderson <[email protected]> > Sent: Wednesday, September 28, 2022 11:12 AM > To: Taylor Simpson <[email protected]>; [email protected] > Cc: [email protected]; [email protected]; [email protected]; Brian Cain > <[email protected]>; Michael Lambert <[email protected]> > 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 <[email protected]> > > --- > > 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 <[email protected]>
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
