[1/2] arm: Add cbz/cbnz macros for arm mode

Message ID 1479456399-18568-1-git-send-email-martin@martin.st
State Rejected
Headers show

Commit Message

Martin Storsjö Nov. 18, 2016, 8:06 a.m.
This allows using the cbz/cbnz instruction in thumb mode, for the
cases where the branch target is close enough.
---
 libavutil/arm/asm.S | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Janne Grunau Nov. 18, 2016, 7:50 p.m. | #1
On 2016-11-18 10:06:38 +0200, Martin Storsjö wrote:
> This allows using the cbz/cbnz instruction in thumb mode, for the
> cases where the branch target is close enough.
> ---
>  libavutil/arm/asm.S | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/libavutil/arm/asm.S b/libavutil/arm/asm.S
> index 4ac0ea2..e66c34c 100644
> --- a/libavutil/arm/asm.S
> +++ b/libavutil/arm/asm.S
> @@ -326,6 +326,18 @@ T       strh            \rt, [\rn]
>  T       sub             \rn, \rn, \rm
>  .endm
>  
> +#if !CONFIG_THUMB
> +.macro  cbz     rd, target
> +        cmp     \rd, #0
> +        beq     \target
> +.endm
> +
> +.macro  cbnz    rd, target
> +        cmp     \rd, #0
> +        bne     \target
> +.endm
> +#endif

I'm not convinced this is a good idea, the thumb instruction works only 
with r0-r7 and has a very limited range while the arm version has no 
such limitations. I.e. unless one tests thumb explicitly it's very easy 
to break thumb. For the few places where it is useful 'T' and 'A' 
prefixed instructions are imo the better way. Using instructions setting 
APSR flags are preferable to cbn?z. This differs on aarch64 since there 
only a some instructions can set flags.

Also I optimized one intended use away.

Janne
Martin Storsjö Nov. 18, 2016, 9:13 p.m. | #2
On Fri, 18 Nov 2016, Janne Grunau wrote:

> On 2016-11-18 10:06:38 +0200, Martin Storsjö wrote:
>> This allows using the cbz/cbnz instruction in thumb mode, for the
>> cases where the branch target is close enough.
>> ---
>>  libavutil/arm/asm.S | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/libavutil/arm/asm.S b/libavutil/arm/asm.S
>> index 4ac0ea2..e66c34c 100644
>> --- a/libavutil/arm/asm.S
>> +++ b/libavutil/arm/asm.S
>> @@ -326,6 +326,18 @@ T       strh            \rt, [\rn]
>>  T       sub             \rn, \rn, \rm
>>  .endm
>>
>> +#if !CONFIG_THUMB
>> +.macro  cbz     rd, target
>> +        cmp     \rd, #0
>> +        beq     \target
>> +.endm
>> +
>> +.macro  cbnz    rd, target
>> +        cmp     \rd, #0
>> +        bne     \target
>> +.endm
>> +#endif
>
> I'm not convinced this is a good idea, the thumb instruction works only
> with r0-r7 and has a very limited range while the arm version has no
> such limitations. I.e. unless one tests thumb explicitly it's very easy
> to break thumb. For the few places where it is useful 'T' and 'A'
> prefixed instructions are imo the better way. Using instructions setting
> APSR flags are preferable to cbn?z. This differs on aarch64 since there
> only a some instructions can set flags.
>
> Also I optimized one intended use away.

Fair enough. Yes, the limited range makes it impractical in many cases, I 
had expected to use it in the vp9 loop filter.

I also noticed now that gas-preprocessor doesn't handle cbz/cbnz 
translation of branch targets (1f etc) for armasm yet, we apparently don't 
use it anywhere yet.

Patch dropped then.

// Martin

Patch

diff --git a/libavutil/arm/asm.S b/libavutil/arm/asm.S
index 4ac0ea2..e66c34c 100644
--- a/libavutil/arm/asm.S
+++ b/libavutil/arm/asm.S
@@ -326,6 +326,18 @@  T       strh            \rt, [\rn]
 T       sub             \rn, \rn, \rm
 .endm
 
+#if !CONFIG_THUMB
+.macro  cbz     rd, target
+        cmp     \rd, #0
+        beq     \target
+.endm
+
+.macro  cbnz    rd, target
+        cmp     \rd, #0
+        bne     \target
+.endm
+#endif
+
 #if HAVE_VFP_ARGS
 ELF     .eabi_attribute 28, 1
 #   define VFP