[FFmpeg-devel,2/6] avcodec/h264: change some labels to be macro-local

Submitted by James Darnley on April 15, 2017, 1:46 a.m.

Details

Message ID 20170415014618.1592-3-jdarnley@obe.tv
State New
Headers show

Commit Message

James Darnley April 15, 2017, 1:46 a.m.
The labels get stripped leading to (slightly) nicer disassembly from
objdump.
---
 libavcodec/x86/h264_idct.asm | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

Ronald S. Bultje April 15, 2017, 12:29 p.m.
Hi,

On Fri, Apr 14, 2017 at 9:46 PM, James Darnley <jdarnley@obe.tv> wrote:

> The labels get stripped leading to (slightly) nicer disassembly from
> objdump.
> ---
>  libavcodec/x86/h264_idct.asm | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/libavcodec/x86/h264_idct.asm b/libavcodec/x86/h264_idct.asm
> index 878ff02..dde40e9 100644
> --- a/libavcodec/x86/h264_idct.asm
> +++ b/libavcodec/x86/h264_idct.asm
> @@ -846,7 +846,7 @@ h264_add8x4_idct_sse2:
>  %macro add16_sse2_cycle 2
>      movzx       r0, word [r4+%2]
>      test        r0, r0
> -    jz .cycle%1end
> +    jz %%skip


So I've thought about it some more. I think I'd first need to understand
what you're doing here and why.

It seems to me that the issue you're trying to address is that when you
look at disassembly (in e.g. a debugger or objdump), it goes from label to
label (where function entry is also a label), and so every function-local
label means disassembly is cut off as a block, right? (Each block then
represents a jump target or loop or something like that.)

And you don't like that, so you're getting rid of the labels, right?

So, if all of this is correct, then I agree that the output of tools like
debugger/objdump is irritating. In fact, it has irritated me forever in any
codec's DSP functions. But it also seems like we're moving away from a de
facto convention if we don't use dot-labels anymore. If we do it for
h264_idct, we should do it everywhere (for consistency). Is that what
people want? Maybe we should follow convention and fix objdump to include
all dot labels in a block if a CLI option is provided?

Ronald
James Darnley April 15, 2017, 2:41 p.m.
On 2017-04-15 14:29, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Apr 14, 2017 at 9:46 PM, James Darnley <jdarnley@obe.tv> wrote:
> 
>> The labels get stripped leading to (slightly) nicer disassembly from
>> objdump.
>> ---
>>  libavcodec/x86/h264_idct.asm | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/libavcodec/x86/h264_idct.asm b/libavcodec/x86/h264_idct.asm
>> index 878ff02..dde40e9 100644
>> --- a/libavcodec/x86/h264_idct.asm
>> +++ b/libavcodec/x86/h264_idct.asm
>> @@ -846,7 +846,7 @@ h264_add8x4_idct_sse2:
>>  %macro add16_sse2_cycle 2
>>      movzx       r0, word [r4+%2]
>>      test        r0, r0
>> -    jz .cycle%1end
>> +    jz %%skip
> 
> 
> So I've thought about it some more. I think I'd first need to understand
> what you're doing here and why.
> 
> It seems to me that the issue you're trying to address is that when you
> look at disassembly (in e.g. a debugger or objdump), it goes from label to
> label (where function entry is also a label), and so every function-local
> label means disassembly is cut off as a block, right? (Each block then
> represents a jump target or loop or something like that.)
> 
> And you don't like that, so you're getting rid of the labels, right?

Yes.  I didn't like that because the function I was looking at had (I
think) 16 labels showing in objdump output.

Strictly speaking, I'm not getting rid of the labels but just changing
them into a format that lets STRIP strip them.  Make will run STRIP to
strip labels that begin ..@ (if configure has determined that your STRIP
supports it).

Usually I don't have a problem with labels representing a loop (or 2
nested ones) because it makes it easy to see where the code jumps back to.

> So, if all of this is correct, then I agree that the output of tools like
> debugger/objdump is irritating. In fact, it has irritated me forever in any
> codec's DSP functions. But it also seems like we're moving away from a de
> facto convention if we don't use dot-labels anymore. If we do it for
> h264_idct, we should do it everywhere (for consistency). Is that what
> people want? Maybe we should follow convention and fix objdump to include
> all dot labels in a block if a CLI option is provided?

... I don't know what to say.
Henrik Gramner April 15, 2017, 3:51 p.m.
What about just using strip -x on the assembly files to discard the
local symbols?

Patch hide | download patch | download mbox

diff --git a/libavcodec/x86/h264_idct.asm b/libavcodec/x86/h264_idct.asm
index 878ff02..dde40e9 100644
--- a/libavcodec/x86/h264_idct.asm
+++ b/libavcodec/x86/h264_idct.asm
@@ -846,7 +846,7 @@  h264_add8x4_idct_sse2:
 %macro add16_sse2_cycle 2
     movzx       r0, word [r4+%2]
     test        r0, r0
-    jz .cycle%1end
+    jz %%skip
     mov        r0d, dword [r1+%1*8]
 %if ARCH_X86_64
     add         r0, r5
@@ -854,7 +854,7 @@  h264_add8x4_idct_sse2:
     add         r0, r0m
 %endif
     call        h264_add8x4_idct_sse2
-.cycle%1end:
+%%skip:
 %if %1 < 7
     add         r2, 64
 %endif
@@ -883,7 +883,7 @@  REP_RET
 %macro add16intra_sse2_cycle 2
     movzx       r0, word [r4+%2]
     test        r0, r0
-    jz .try%1dc
+    jz %%trydc
     mov        r0d, dword [r1+%1*8]
 %if ARCH_X86_64
     add         r0, r7
@@ -891,11 +891,11 @@  REP_RET
     add         r0, r0m
 %endif
     call        h264_add8x4_idct_sse2
-    jmp .cycle%1end
-.try%1dc:
+    jmp %%skip
+%%trydc:
     movsx       r0, word [r2   ]
     or         r0w, word [r2+32]
-    jz .cycle%1end
+    jz %%skip
     mov        r0d, dword [r1+%1*8]
 %if ARCH_X86_64
     add         r0, r7
@@ -903,7 +903,7 @@  REP_RET
     add         r0, r0m
 %endif
     call        h264_idct_dc_add8_mmxext
-.cycle%1end:
+%%skip:
 %if %1 < 7
     add         r2, 64
 %endif
@@ -930,7 +930,7 @@  REP_RET
 %macro add8_sse2_cycle 2
     movzx       r0, word [r4+%2]
     test        r0, r0
-    jz .try%1dc
+    jz %%trydc
 %if ARCH_X86_64
     mov        r0d, dword [r1+(%1&1)*8+64*(1+(%1>>1))]
     add         r0, [r7]
@@ -940,11 +940,11 @@  REP_RET
     add         r0, dword [r1+(%1&1)*8+64*(1+(%1>>1))]
 %endif
     call        h264_add8x4_idct_sse2
-    jmp .cycle%1end
-.try%1dc:
+    jmp %%cycle_end
+%%trydc:
     movsx       r0, word [r2   ]
     or         r0w, word [r2+32]
-    jz .cycle%1end
+    jz %%cycle_end
 %if ARCH_X86_64
     mov        r0d, dword [r1+(%1&1)*8+64*(1+(%1>>1))]
     add         r0, [r7]
@@ -954,7 +954,7 @@  REP_RET
     add         r0, dword [r1+(%1&1)*8+64*(1+(%1>>1))]
 %endif
     call        h264_idct_dc_add8_mmxext
-.cycle%1end:
+%%cycle_end:
 %if %1 == 1
     add         r2, 384+64
 %elif %1 < 3