[FFmpeg-devel] [PATCH] h264: mark xmm registers as clobbered in h264 qpel sse functions

[Michael Niedermayer]

On Wed, Oct 27, 2010 at 09:26:10PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Oct 27, 2010 at 9:23 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
> > On Mon, Oct 25, 2010 at 6:58 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
> >> On Mon, Oct 25, 2010 at 6:07 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
> >>> On Mon, Oct 25, 2010 at 5:54 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >>>> On Mon, Oct 25, 2010 at 04:43:27PM -0200, Ramiro Polla wrote:
> >>>>> On Thu, Oct 7, 2010 at 8:32 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >>>>> > On Thu, Oct 07, 2010 at 06:26:59PM -0300, Ramiro Polla wrote:
> >>>>> >> $subj, fixes h264 for win64
> >>>>> >
> >>>>> >> ?h264_qpel_mmx.c | ? 21 +++++++++++++++++++++
> >>>>> >> ?1 file changed, 21 insertions(+)
> >>>>> >> f7a94ad1d2ef7ffec9ce1a219ad9c9c4efc0e8d7 ?xmm_clobbers_h264_qpel_mmx.diff
> >>>>> >> Index: libavcodec/x86/h264_qpel_mmx.c
> >>>>> >> ===================================================================
> >>>>> >> --- libavcodec/x86/h264_qpel_mmx.c ? ?(revision 25401)
> >>>>> >> +++ libavcodec/x86/h264_qpel_mmx.c ? ?(working copy)
> >>>>> >> @@ -677,6 +677,10 @@
> >>>>> >> ? ? ? ? ?: "D"((x86_reg)src2Stride), "S"((x86_reg)dstStride),\
> >>>>> >> ? ? ? ? ? ?"m"(ff_pw_5), "m"(ff_pw_16)\
> >>>>> >> ? ? ? ? ?: "memory"\
> >>>>> >> + ? ? ? ? ?XMM_CLOBBERS(, "%xmm0", "%xmm1", "%xmm2", "%xmm3", \
> >>>>> >> + ? ? ? ? ? ? ? ? ? ? ? ? "%xmm4", "%xmm5", "%xmm6", "%xmm7", \
> >>>>> >> + ? ? ? ? ? ? ? ? ? ? ? ? "%xmm8", "%xmm9", "%xmm10", "%xmm11", \
> >>>>> >> + ? ? ? ? ? ? ? ? ? ? ? ? "%xmm12", "%xmm13", "%xmm14", "%xmm15") \
> >>>>> >> ? ? ?);\
> >>>>> >> ?}
> >>>>> >> ?#else // ARCH_X86_64
> >>>>> >
> >>>>> >> @@ -699,6 +703,7 @@
> >>>>> >> ? ? ? ? ?"pxor %%xmm7, %%xmm7 ? ? ? ?\n\t"\
> >>>>> >> ? ? ? ? ?"movdqa %0, %%xmm6 ? ? ? ? ?\n\t"\
> >>>>> >> ? ? ? ? ?:: "m"(ff_pw_5)\
> >>>>> >> + ? ? ? ?XMM_CLOBBERS_ONLY("%xmm6", "%xmm7") \
> >>>>> >> ? ? ?);\
> >>>>> >> ? ? ?do{\
> >>>>> >
> >>>>> >
> >>>>> > this is wrong
> >>>>> > 6/7 are read after the asm
> >>>>> > correct is to either merge the asm blocks or to put manual store restore code
> >>>>> > for the xmm registers there under appropriate ifdef
> >>>>>
> >>>>> Patch attached merging other cases in the same file. I'm not really
> >>>>> sure if those "cmp" are ok or if they should be suffixed, but that
> >>>>> seems to have worked.
> >>>>
> >>>>
> >>>> they need suffixes
> >>>
> >>> as in attached?
> >>
> >> Hm, obviously that "int $3" shouldn't be there.
> >
> > ping
> 
> I already said LGTM tot he last version (and made the same cmp -> cmpl
> remark on IRC also :-p). Michael, would you like to co-review these
> changes or is my review enough?

1 review should generally be enough for changes of this simplicity but i
took a quick look too now.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101028/c971436b/attachment.pgp>