> -----Original Message-----
> From: Matheus Tavares Bernardino <[email protected]>
> Sent: Thursday, October 20, 2022 10:24 AM
> To: Taylor Simpson <[email protected]>
> Cc: [email protected]; [email protected]; Brian Cain <[email protected]>;
> [email protected]; [email protected]; Matheus Bernardino (QUIC)
> <[email protected]>; [email protected]
> Subject: Re: [PATCH 6/8] Hexagon (target/hexagon) Add overrides for
> various forms of jump
>
> Taylor Simpson <[email protected]> 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
