| Message ID | 20190913101714.29019-3-svens@stackframe.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HPPA tcg fixes | expand |
Hi Sven, On 9/13/19 12:17 PM, Sven Schnelle wrote: > nullify_over() calls brcond which destroys all temporaries. > > Signed-off-by: Sven Schnelle <svens@stackframe.org> > --- > target/hppa/translate.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/target/hppa/translate.c b/target/hppa/translate.c > index b12525d535..c1b2822f60 100644 > --- a/target/hppa/translate.c > +++ b/target/hppa/translate.c > @@ -3404,10 +3404,6 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c, > TCGv_reg mask, tmp, shift, dest; > unsigned msb = 1U << (len - 1); > > - if (c) { > - nullify_over(ctx); > - } > - > dest = dest_gpr(ctx, rt); > shift = tcg_temp_new(); > tmp = tcg_temp_new(); > @@ -3440,11 +3436,17 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c, > > static bool trans_depw_sar(DisasContext *ctx, arg_depw_sar *a) > { > + if (a->c) { > + nullify_over(ctx); > + } > return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r)); > } > > static bool trans_depwi_sar(DisasContext *ctx, arg_depwi_sar *a) > { > + if (a->c) { > + nullify_over(ctx); > + } I don't see how this patch helps or change anything, isn't it the same? You clean in the caller rather than the callee. > return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_const(ctx, a->i)); > } > >
Hi Philippe, On Fri, Sep 13, 2019 at 12:58:14PM +0200, Philippe Mathieu-Daudé wrote: > Hi Sven, > > On 9/13/19 12:17 PM, Sven Schnelle wrote: > > nullify_over() calls brcond which destroys all temporaries. > > > > Signed-off-by: Sven Schnelle <svens@stackframe.org> > > --- > > target/hppa/translate.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/target/hppa/translate.c b/target/hppa/translate.c > > index b12525d535..c1b2822f60 100644 > > --- a/target/hppa/translate.c > > +++ b/target/hppa/translate.c > > @@ -3404,10 +3404,6 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c, > > TCGv_reg mask, tmp, shift, dest; > > unsigned msb = 1U << (len - 1); > > > > - if (c) { > > - nullify_over(ctx); > > - } > > - > > dest = dest_gpr(ctx, rt); > > shift = tcg_temp_new(); > > tmp = tcg_temp_new(); > > @@ -3440,11 +3436,17 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c, > > > > static bool trans_depw_sar(DisasContext *ctx, arg_depw_sar *a) > > { > > + if (a->c) { > > + nullify_over(ctx); > > + } > > return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r)); > > } > > > > static bool trans_depwi_sar(DisasContext *ctx, arg_depwi_sar *a) > > { > > + if (a->c) { > > + nullify_over(ctx); > > + } > > I don't see how this patch helps or change anything, isn't it the same? > You clean in the caller rather than the callee. The Problem is that load_gpr()/load_const() allocate a temporary, which gets destroyed in do_depw_sar() when nullify_over() is called. If we move the nullify_over() before doing the load_gpr()/load_const() this doesn't happen. > > return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_const(ctx, a->i)); > > } > > > > Regards Sven
On 9/13/19 1:08 PM, Sven Schnelle wrote: > Hi Philippe, > > On Fri, Sep 13, 2019 at 12:58:14PM +0200, Philippe Mathieu-Daudé wrote: >> Hi Sven, >> >> On 9/13/19 12:17 PM, Sven Schnelle wrote: >>> nullify_over() calls brcond which destroys all temporaries. >>> >>> Signed-off-by: Sven Schnelle <svens@stackframe.org> >>> --- >>> target/hppa/translate.c | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/target/hppa/translate.c b/target/hppa/translate.c >>> index b12525d535..c1b2822f60 100644 >>> --- a/target/hppa/translate.c >>> +++ b/target/hppa/translate.c >>> @@ -3404,10 +3404,6 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c, >>> TCGv_reg mask, tmp, shift, dest; >>> unsigned msb = 1U << (len - 1); >>> >>> - if (c) { >>> - nullify_over(ctx); >>> - } >>> - >>> dest = dest_gpr(ctx, rt); >>> shift = tcg_temp_new(); >>> tmp = tcg_temp_new(); >>> @@ -3440,11 +3436,17 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c, >>> >>> static bool trans_depw_sar(DisasContext *ctx, arg_depw_sar *a) >>> { >>> + if (a->c) { >>> + nullify_over(ctx); >>> + } >>> return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r)); >>> } >>> >>> static bool trans_depwi_sar(DisasContext *ctx, arg_depwi_sar *a) >>> { >>> + if (a->c) { >>> + nullify_over(ctx); >>> + } >> >> I don't see how this patch helps or change anything, isn't it the same? >> You clean in the caller rather than the callee. > > The Problem is that load_gpr()/load_const() allocate a temporary, which > gets destroyed in do_depw_sar() when nullify_over() is called. If we > move the nullify_over() before doing the load_gpr()/load_const() this > doesn't happen. Ah! The 'val' argument... I missed that, thanks! Maybe we can add a comment to make it clearer: if (a->c) { /* * nullify here to not free the load_gpr() arg before * calling depw_sar. */ nullify_over(ctx); } return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r)); (also in the other function). Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_const(ctx, a->i)); >>> } >>> >>> > > Regards > Sven >
diff --git a/target/hppa/translate.c b/target/hppa/translate.c index b12525d535..c1b2822f60 100644 --- a/target/hppa/translate.c +++ b/target/hppa/translate.c @@ -3404,10 +3404,6 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c, TCGv_reg mask, tmp, shift, dest; unsigned msb = 1U << (len - 1); - if (c) { - nullify_over(ctx); - } - dest = dest_gpr(ctx, rt); shift = tcg_temp_new(); tmp = tcg_temp_new(); @@ -3440,11 +3436,17 @@ static bool do_depw_sar(DisasContext *ctx, unsigned rt, unsigned c, static bool trans_depw_sar(DisasContext *ctx, arg_depw_sar *a) { + if (a->c) { + nullify_over(ctx); + } return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_gpr(ctx, a->r)); } static bool trans_depwi_sar(DisasContext *ctx, arg_depwi_sar *a) { + if (a->c) { + nullify_over(ctx); + } return do_depw_sar(ctx, a->t, a->c, a->nz, a->clen, load_const(ctx, a->i)); }
nullify_over() calls brcond which destroys all temporaries. Signed-off-by: Sven Schnelle <svens@stackframe.org> --- target/hppa/translate.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)