[Qemu-devel] [PATCH] decodetree: Propagate return value from translate subroutines

[Qemu-devel] [PATCH] decodetree: Propagate return value from translate subroutines

[Richard Henderson]

Allow the translate subroutines to return false for invalid insns.

At present we can of course invoke an invalid insn exception from within
the translate subroutine, but in the short term this consolidates code.
In the long term it would allow the decodetree language to support
overlapping patterns for ISA extensions.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Since this makes an ABI change to the translate functions called by the
decode function, let's make it now before there are any in-tree users.

My SVE branch over-decodes in quite a lot of cases -- e.g. things like
the 2-bit size field must be 1-3 for fp operands, and so size==0 is
unallocated.  Returning false for these cases allows the actual call
to unallocated_encoding to be done in one place instead of hundreds.

Longer term, I'm thinking of how to handle decode of overlapping ISA
extensions.  One could allow (specific) overlapping patterns and
prioritize them in some way (e.g. first in file is first matched).
My thought is that trans_insn_a would check a cpu feature bit and
return false if not enabled.  Then trans_insn_b would be given its
chance to handle the insn.


r~
---
 scripts/decodetree.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/decodetree.py b/scripts/decodetree.py
index 6a33f8f8dd..41301c84aa 100755
--- a/scripts/decodetree.py
+++ b/scripts/decodetree.py
@@ -461,7 +461,7 @@ class Pattern(General):
         global translate_prefix
         output('typedef ', self.base.base.struct_name(),
                ' arg_', self.name, ';\n')
-        output(translate_scope, 'void ', translate_prefix, '_', self.name,
+        output(translate_scope, 'bool ', translate_prefix, '_', self.name,
                '(DisasContext *ctx, arg_', self.name,
                ' *a, ', insntype, ' insn);\n')
 
@@ -474,9 +474,8 @@ class Pattern(General):
             output(ind, self.base.extract_name(), '(&u.f_', arg, ', insn);\n')
         for n, f in self.fields.items():
             output(ind, 'u.f_', arg, '.', n, ' = ', f.str_extract(), ';\n')
-        output(ind, translate_prefix, '_', self.name,
+        output(ind, 'return ', translate_prefix, '_', self.name,
                '(ctx, &u.f_', arg, ', insn);\n')
-        output(ind, 'return true;\n')
 # end Pattern
 
 
-- 
2.14.3

[Qemu-devel] [PATCH] decodetree: Propagate return value from translate subroutines

[Richard Henderson]

Allow the translate subroutines to return false for invalid insns.

At present we can of course invoke an invalid insn exception from within
the translate subroutine, but in the short term this consolidates code.
In the long term it would allow the decodetree language to support
overlapping patterns for ISA extensions.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Since this makes an ABI change to the translate functions called by the
decode function, let's make it now before there are any in-tree users.

My SVE branch over-decodes in quite a lot of cases -- e.g. things like
the 2-bit size field must be 1-3 for fp operands, and so size==0 is
unallocated.  Returning false for these cases allows the actual call
to unallocated_encoding to be done in one place instead of hundreds.

Longer term, I'm thinking of how to handle decode of overlapping ISA
extensions.  One could allow (specific) overlapping patterns and
prioritize them in some way (e.g. first in file is first matched).
My thought is that trans_insn_a would check a cpu feature bit and
return false if not enabled.  Then trans_insn_b would be given its
chance to handle the insn.


r~
---
 scripts/decodetree.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/decodetree.py b/scripts/decodetree.py
index 6a33f8f8dd..41301c84aa 100755
--- a/scripts/decodetree.py
+++ b/scripts/decodetree.py
@@ -461,7 +461,7 @@ class Pattern(General):
         global translate_prefix
         output('typedef ', self.base.base.struct_name(),
                ' arg_', self.name, ';\n')
-        output(translate_scope, 'void ', translate_prefix, '_', self.name,
+        output(translate_scope, 'bool ', translate_prefix, '_', self.name,
                '(DisasContext *ctx, arg_', self.name,
                ' *a, ', insntype, ' insn);\n')
 
@@ -474,9 +474,8 @@ class Pattern(General):
             output(ind, self.base.extract_name(), '(&u.f_', arg, ', insn);\n')
         for n, f in self.fields.items():
             output(ind, 'u.f_', arg, '.', n, ' = ', f.str_extract(), ';\n')
-        output(ind, translate_prefix, '_', self.name,
+        output(ind, 'return ', translate_prefix, '_', self.name,
                '(ctx, &u.f_', arg, ', insn);\n')
-        output(ind, 'return true;\n')
 # end Pattern
 
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH] decodetree: Propagate return value from translate subroutines

[Peter Maydell]

On 28 February 2018 at 16:48, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Allow the translate subroutines to return false for invalid insns.
>
> At present we can of course invoke an invalid insn exception from within
> the translate subroutine, but in the short term this consolidates code.
> In the long term it would allow the decodetree language to support
> overlapping patterns for ISA extensions.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Since this makes an ABI change to the translate functions called by the
> decode function, let's make it now before there are any in-tree users.
>
> My SVE branch over-decodes in quite a lot of cases -- e.g. things like
> the 2-bit size field must be 1-3 for fp operands, and so size==0 is
> unallocated.  Returning false for these cases allows the actual call
> to unallocated_encoding to be done in one place instead of hundreds.
>
> Longer term, I'm thinking of how to handle decode of overlapping ISA
> extensions.  One could allow (specific) overlapping patterns and
> prioritize them in some way (e.g. first in file is first matched).
> My thought is that trans_insn_a would check a cpu feature bit and
> return false if not enabled.  Then trans_insn_b would be given its
> chance to handle the insn.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I'll put this into target-arm.next, unless you'd prefer it to go
in via some other route.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] decodetree: Propagate return value from translate subroutines

[Richard Henderson]

On 03/01/2018 03:59 AM, Peter Maydell wrote:
> On 28 February 2018 at 16:48, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> Allow the translate subroutines to return false for invalid insns.
>>
>> At present we can of course invoke an invalid insn exception from within
>> the translate subroutine, but in the short term this consolidates code.
>> In the long term it would allow the decodetree language to support
>> overlapping patterns for ISA extensions.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>
>> Since this makes an ABI change to the translate functions called by the
>> decode function, let's make it now before there are any in-tree users.
>>
>> My SVE branch over-decodes in quite a lot of cases -- e.g. things like
>> the 2-bit size field must be 1-3 for fp operands, and so size==0 is
>> unallocated.  Returning false for these cases allows the actual call
>> to unallocated_encoding to be done in one place instead of hundreds.
>>
>> Longer term, I'm thinking of how to handle decode of overlapping ISA
>> extensions.  One could allow (specific) overlapping patterns and
>> prioritize them in some way (e.g. first in file is first matched).
>> My thought is that trans_insn_a would check a cpu feature bit and
>> return false if not enabled.  Then trans_insn_b would be given its
>> chance to handle the insn.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I'll put this into target-arm.next, unless you'd prefer it to go
> in via some other route.

That's fine.  Thanks!


r~