diff mbox series

[v4,3/5] tcg/sparc: Use the constant pool for 64-bit constants

Message ID 20220204070011.573941-4-richard.henderson@linaro.org
State Superseded
Headers show
Series tcg/sparc: Unaligned access for user-only | expand

Commit Message

Richard Henderson Feb. 4, 2022, 7 a.m. UTC 
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/sparc/tcg-target.c.inc | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Peter Maydell Feb. 4, 2022, 6:18 p.m. UTC | #1 
On Fri, 4 Feb 2022 at 07:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tcg/sparc/tcg-target.c.inc | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc
> index 6349f750cc..47bdf314a0 100644
> --- a/tcg/sparc/tcg-target.c.inc
> +++ b/tcg/sparc/tcg-target.c.inc
> @@ -332,6 +332,13 @@ static bool patch_reloc(tcg_insn_unit *src_rw, int type,
>          insn &= ~INSN_OFF19(-1);
>          insn |= INSN_OFF19(pcrel);
>          break;
> +    case R_SPARC_13:
> +        if (!check_fit_ptr(value, 13)) {
> +            return false;
> +        }

This code seems to contemplate that the offset might not fit
into the 13-bit immediate field (unlike the other two reloc
cases in this function, which just assert() that it fits)...

> +        insn &= ~INSN_IMM13(-1);
> +        insn |= INSN_IMM13(value);
> +        break;
>      default:
>          g_assert_not_reached();
>      }
> @@ -469,6 +476,14 @@ static void tcg_out_movi_int(TCGContext *s, TCGType type, TCGReg ret,
>          return;
>      }
>
> +    /* Use the constant pool, if possible. */
> +    if (!in_prologue && USE_REG_TB) {
> +        new_pool_label(s, arg, R_SPARC_13, s->code_ptr,
> +                       tcg_tbrel_diff(s, NULL));
> +        tcg_out32(s, LDX | INSN_RD(ret) | INSN_RS1(TCG_REG_TB));
> +        return;
> +    }

...but this code doesn't seem to have any mechanism for
falling back to something else if it won't fit.

> +
>      /* A 64-bit constant decomposed into 2 32-bit pieces.  */
>      if (check_fit_i32(lo, 13)) {
>          hi = (arg - lo) >> 32;
> --
> 2.25.1

thanks
-- PMM
Richard Henderson Feb. 4, 2022, 8:41 p.m. UTC | #2 
On 2/5/22 05:18, Peter Maydell wrote:
> On Fri, 4 Feb 2022 at 07:53, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   tcg/sparc/tcg-target.c.inc | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc
>> index 6349f750cc..47bdf314a0 100644
>> --- a/tcg/sparc/tcg-target.c.inc
>> +++ b/tcg/sparc/tcg-target.c.inc
>> @@ -332,6 +332,13 @@ static bool patch_reloc(tcg_insn_unit *src_rw, int type,
>>           insn &= ~INSN_OFF19(-1);
>>           insn |= INSN_OFF19(pcrel);
>>           break;
>> +    case R_SPARC_13:
>> +        if (!check_fit_ptr(value, 13)) {
>> +            return false;
>> +        }
> 
> This code seems to contemplate that the offset might not fit
> into the 13-bit immediate field (unlike the other two reloc
> cases in this function, which just assert() that it fits)...

Ooo, thanks for noticing.  The other entries have not been updated for changes to tcg 
relocations.  They should be returning false instead of asserting.

Returning false here tells generic code the relocation didn't fit, and to restart the TB 
with half of the number of guest instructions.

>> +    /* Use the constant pool, if possible. */
>> +    if (!in_prologue && USE_REG_TB) {
>> +        new_pool_label(s, arg, R_SPARC_13, s->code_ptr,
>> +                       tcg_tbrel_diff(s, NULL));
>> +        tcg_out32(s, LDX | INSN_RD(ret) | INSN_RS1(TCG_REG_TB));
>> +        return;
>> +    }
> 
> ...but this code doesn't seem to have any mechanism for
> falling back to something else if it won't fit.

It will fit, because we'll keep trying with smaller TBs until it does.  If for some reason 
a target generates more than 8k for a single guest insn... it will go boom.


r~
Peter Maydell Feb. 4, 2022, 10:20 p.m. UTC | #3 
On Fri, 4 Feb 2022 at 20:41, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/5/22 05:18, Peter Maydell wrote:
> > On Fri, 4 Feb 2022 at 07:53, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   tcg/sparc/tcg-target.c.inc | 15 +++++++++++++++
> >>   1 file changed, 15 insertions(+)
> >>
> >> diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc
> >> index 6349f750cc..47bdf314a0 100644
> >> --- a/tcg/sparc/tcg-target.c.inc
> >> +++ b/tcg/sparc/tcg-target.c.inc
> >> @@ -332,6 +332,13 @@ static bool patch_reloc(tcg_insn_unit *src_rw, int type,
> >>           insn &= ~INSN_OFF19(-1);
> >>           insn |= INSN_OFF19(pcrel);
> >>           break;
> >> +    case R_SPARC_13:
> >> +        if (!check_fit_ptr(value, 13)) {
> >> +            return false;
> >> +        }
> >
> > This code seems to contemplate that the offset might not fit
> > into the 13-bit immediate field (unlike the other two reloc
> > cases in this function, which just assert() that it fits)...
>
> Ooo, thanks for noticing.  The other entries have not been updated for changes to tcg
> relocations.  They should be returning false instead of asserting.
>
> Returning false here tells generic code the relocation didn't fit, and to restart the TB
> with half of the number of guest instructions.
>
> >> +    /* Use the constant pool, if possible. */
> >> +    if (!in_prologue && USE_REG_TB) {
> >> +        new_pool_label(s, arg, R_SPARC_13, s->code_ptr,
> >> +                       tcg_tbrel_diff(s, NULL));
> >> +        tcg_out32(s, LDX | INSN_RD(ret) | INSN_RS1(TCG_REG_TB));
> >> +        return;
> >> +    }
> >
> > ...but this code doesn't seem to have any mechanism for
> > falling back to something else if it won't fit.
>
> It will fit, because we'll keep trying with smaller TBs until it does.  If for some reason
> a target generates more than 8k for a single guest insn... it will go boom.

Ah, I see. Then for this patch
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
diff mbox series

Patch

diff --git a/tcg/sparc/tcg-target.c.inc b/tcg/sparc/tcg-target.c.inc
index 6349f750cc..47bdf314a0 100644
--- a/tcg/sparc/tcg-target.c.inc
+++ b/tcg/sparc/tcg-target.c.inc
@@ -332,6 +332,13 @@  static bool patch_reloc(tcg_insn_unit *src_rw, int type,
         insn &= ~INSN_OFF19(-1);
         insn |= INSN_OFF19(pcrel);
         break;
+    case R_SPARC_13:
+        if (!check_fit_ptr(value, 13)) {
+            return false;
+        }
+        insn &= ~INSN_IMM13(-1);
+        insn |= INSN_IMM13(value);
+        break;
     default:
         g_assert_not_reached();
     }
@@ -469,6 +476,14 @@  static void tcg_out_movi_int(TCGContext *s, TCGType type, TCGReg ret,
         return;
     }
 
+    /* Use the constant pool, if possible. */
+    if (!in_prologue && USE_REG_TB) {
+        new_pool_label(s, arg, R_SPARC_13, s->code_ptr,
+                       tcg_tbrel_diff(s, NULL));
+        tcg_out32(s, LDX | INSN_RD(ret) | INSN_RS1(TCG_REG_TB));
+        return;
+    }
+
     /* A 64-bit constant decomposed into 2 32-bit pieces.  */
     if (check_fit_i32(lo, 13)) {
         hi = (arg - lo) >> 32;