> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathb...@quicinc.com>
> Sent: Thursday, October 20, 2022 10:24 AM
> To: Taylor Simpson <tsimp...@quicinc.com>
> Cc: a...@rev.ng; a...@rev.ng; Brian Cain <bc...@quicinc.com>;
> phi...@linaro.org; qemu-devel@nongnu.org; Matheus Bernardino (QUIC)
> <quic_mathb...@quicinc.com>; richard.hender...@linaro.org
> Subject: Re: [PATCH 6/8] Hexagon (target/hexagon) Add overrides for
> various forms of jump
>
> Taylor Simpson <tsimp...@quicinc.com> wrote:
> > diff --git a/target/hexagon/gen_tcg.h b/target/hexagon/gen_tcg.h index
> > b56b216110..dbafcae2de 100644
> > --- a/target/hexagon/gen_tcg.h
> > +++ b/target/hexagon/gen_tcg.h
> >
> > +#define fGEN_TCG_J4_cmplt_f_jumpnv_t(SHORTCODE) \
> > + gen_cmp_jumpnv(ctx, pkt, TCG_COND_GE, NsN, RtV, riV) #define
> > +fGEN_TCG_J4_cmplt_f_jumpnv_nt(SHORTCODE) \
> > + gen_cmp_jumpnv(ctx, pkt, TCG_COND_GE, NsN, RtV, riV)
> > +
>
> Nitpick: any reason not to place these two !COND variants...
>
> > +
> > +#define fGEN_TCG_J4_cmplt_t_jumpnv_t(SHORTCODE) \
> > + gen_cmp_jumpnv(ctx, pkt, TCG_COND_LT, NsN, RtV, riV) #define
> > +fGEN_TCG_J4_cmplt_t_jumpnv_nt(SHORTCODE) \
> > + gen_cmp_jumpnv(ctx, pkt, TCG_COND_LT, NsN, RtV, riV)
> > +
>
> ...below these COND variants, like you did in the other insns?
>
Good catch, I will change this.
> > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> > 6e494c0bd8..fba76d3b38 100644
> > --- a/target/hexagon/genptr.c
> > +++ b/target/hexagon/genptr.c
> > @@ -456,6 +456,35 @@ static TCGv gen_8bitsof(TCGv result, TCGv value)
> > return result;
> > }
> >
> > +static void gen_write_new_pc_addr(DisasContext *ctx, Packet *pkt,
> > + TCGv addr, TCGv pred) {
> > + TCGLabel *pred_false = NULL;
> > + if (pkt->pkt_has_multi_cof) {
> > + if (pred != NULL) {
> > + pred_false = gen_new_label();
> > + tcg_gen_brcondi_tl(TCG_COND_EQ, pred, 0, pred_false);
> > + }
> > + /* If there are multiple branches in a packet, ignore the second
> > one */
> > + tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
> > + hex_branch_taken, tcg_constant_tl(0),
> > + hex_gpr[HEX_REG_PC], addr);
> > + tcg_gen_movi_tl(hex_branch_taken, 1);
> > + if (pred != NULL) {
> > + gen_set_label(pred_false);
> > + }
> > + } else {
> > + if (pred != NULL) {
> > + pred_false = gen_new_label();
> > + tcg_gen_brcondi_tl(TCG_COND_EQ, pred, 0, pred_false);
> > + }
> > + tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
> > + if (pred != NULL) {
> > + gen_set_label(pred_false);
> > + }
> > + }
>
> Another nitpick: we could possibly extract the common code out of the if-
> else branches for clarity (but note my other comment about this function at
> patch 7):
>
> static void gen_write_new_pc_addr(DisasContext *ctx, Packet *pkt,
> TCGv addr, TCGv pred) {
> TCGLabel *pred_false = NULL;
> if (pred != NULL) {
> pred_false = gen_new_label();
> tcg_gen_brcondi_tl(TCG_COND_EQ, pred, 0, pred_false);
> }
> if (pkt->pkt_has_multi_cof) {
> /* If there are multiple branches in a packet, ignore the second one
> */
> tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
> hex_branch_taken, tcg_constant_tl(0),
> hex_gpr[HEX_REG_PC], addr);
> tcg_gen_movi_tl(hex_branch_taken, 1);
> } else {
> tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
> }
> if (pred != NULL) {
> gen_set_label(pred_false);
> }
> }
Yes, this will be easier to read. I'll change it.
> The rest of the patch LGTM.
Thanks,
Taylor
