Bug 1392465 - pygame tests fail on ppc64le because vec_perm() has big-endian semantics on PPC64LE
Summary: pygame tests fail on ppc64le because vec_perm() has big-endian semantics on P...
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: SDL
Version: 26
Hardware: ppc64le
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Petr Pisar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
: 1392887 (view as bug list)
Depends On:
Blocks: PPCTracker
TreeView+ depends on / blocked
 
Reported: 2016-11-07 15:00 UTC by Dennis Gilmore
Modified: 2017-12-04 13:59 UTC (History)
21 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2017-12-04 13:59:16 UTC


Attachments (Terms of Use)
vector test code (1.35 KB, text/x-csrc)
2017-07-31 14:35 UTC, Menanteau Guy
no flags Details
correct result with vector char (1.21 KB, text/x-csrc)
2017-09-06 09:55 UTC, Menanteau Guy
no flags Details
reorder bytes (1.36 KB, text/x-csrc)
2017-09-06 09:56 UTC, Menanteau Guy
no flags Details
proposed patch (3.25 KB, patch)
2017-09-06 10:02 UTC, Menanteau Guy
no flags Details | Diff
0001-Re-enable-tests-on-ppc64le.patch (1.94 KB, patch)
2017-09-30 19:20 UTC, Sergio Monteiro Basto
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Red Hat Bugzilla 1487174 None CLOSED backport commit to compile opencv with ppc64le 2019-06-04 08:55 UTC

Description Dennis Gilmore 2016-11-07 15:00:35 UTC
Description of problem:
FAILED (failures=4)
+ :
+ PYTHONPATH=/builddir/build/BUILDROOT/pygame-1.9.1-22.fc26.20150926.ppc64le/usr/lib64/python2.7/site-packages
+ /usr/bin/python2 test/image_test.py
........F.
======================================================================
FAIL: make sure the color key is not changed when saving.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/image_test.py", line 284, in test_save_colorkey
    self.assertEqual(p1, s2.get_at((0,0)))
AssertionError: (23, 23, 23, 255) != (23, 23, 255, 23)
----------------------------------------------------------------------
Ran 10 tests in 0.600s
FAILED (failures=1)

Comment 1 Michael J Gruber 2016-11-07 15:58:53 UTC
That's an endianness problem fixed quite a while ago:

https://bitbucket.org/pygame/pygame/issues/39

But the fix seems to be in pygame 1.9.2 only, afaics.

Comment 2 Dan Horák 2016-11-08 12:29:29 UTC
*** Bug 1392887 has been marked as a duplicate of this bug. ***

Comment 3 Hans de Goede 2016-11-08 12:52:51 UTC
(In reply to Michael J Gruber from comment #1)
> That's an endianness problem fixed quite a while ago:
> 
> https://bitbucket.org/pygame/pygame/issues/39
> 
> But the fix seems to be in pygame 1.9.2 only, afaics.

That is about a big-endian issue, where as here we've a failure on ppc64le, so little-endian and the test works fine on x86 so it is not just an endianness issue.

Comment 4 Dan Horák 2016-11-08 12:55:49 UTC
Maybe it's always treating ppc64 as big endian ...

Comment 5 Hans de Goede 2016-11-09 08:37:20 UTC
(In reply to Dan Horák from comment #4)
> Maybe it's always treating ppc64 as big endian ...

I've checked pygame, SDL, SDL_image and libpng for something like this (using case insensitive grep for PPC / POWERPC) and I could not find anything like this.

Comment 6 Fedora End Of Life 2017-02-28 10:34:11 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 26 development cycle.
Changing version to '26'.

Comment 7 Gwyn Ciesla 2017-04-10 14:58:11 UTC
Updated to 1.9.3, and this still happens.

Comment 8 Michael J Gruber 2017-04-11 08:55:13 UTC
It appears that pygame folks could need a helping hand for testing on big endian to confirm that the proposed fix works:

https://bitbucket.org/pygame/pygame/issues/211/red-and-green-channels-swapped-when-saving

(It turned out that the old fix from issue 39 was not quite correct.)

Comment 9 Dan Horák 2017-04-11 10:02:32 UTC
The issue #211 talks about incomplete big endian fix, which is now long time committed. Our problem is not on big endians (ppc64, s390x), it's all OK there, but with the little endian flavour of ppc (aka ppc64le).

Comment 10 Menanteau Guy 2017-07-31 14:35 UTC
Created attachment 1307110 [details]
vector test code

I looked to the failed test on ppc64le and I traced a strange behaviour of vector used in SDL.
SavePNG() function of pygame somewhere reach ConvertAltivec32to32_prefetch() function of SDL.

ConvertAltivec32to32_prefetch() of SDL-1.2.15/src/video/SDL_blit_N.c
uses altivec vectors thru this code:
            src += 4;
            width -= 4;
            vbits = vec_perm(vbits, voverflow, valigner);  /* src is ready. */
            vbits = vec_perm(vbits, vzero, vpermute);  /* swizzle it. */
            vec_st(vbits, 0, dst);  /* store it back out. */
            dst += 4;
            vbits = voverflow;


I am puzzled by the behaviour of vec_perm where it don't return the result I expect.

I highlighted the suspect behaviour of vec_perm in ppc64le by writting a test code: test.c (see attachment)

I compiled it with the following options (same as those used to build SDL).
gcc -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mcpu=power8 -mtune=power8 -I./include -D_GNU_SOURCE=1 -fvisibility=hidden -maltivec -pthread -I/usr/include/kde/artsc -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -D_REENTRANT -DXTHREADS -D_REENTRANT -DHAVE_LINUX_VERSION_H -Wall test.c  -fPIC -DPIC -o test

When I ran ./test, I got:
 vperm[0] 0
 vperm[0] 3
 vperm[0] 2
 vperm[0] 1
 vbits[0] 0x112233
 resulting vbits[0] 0x22110033
 vbits[0] expected 0x00332211

Do you agree vec_perm does not return the correct result in this case ?

packages version:
pygame-1.9.3-1
SDL-1.2.15-24
gcc-7.1.1-6
glibc-2.25.90-29

Comment 11 Menanteau Guy 2017-09-06 09:49:37 UTC
Bill Schmidt commented this behaviour as follow:
The LE transformation for vec_perm has an implicit assumption that the permutation is being used to reorder vector elements (in this case 4-byte integer word elements), not to reorder bytes within those elements.  Although this is legal behavior, it is not anticipated by the transformation performed by the compilers.  (CCing reps from XLC and LLVM compilers for awareness.)

Note that if vec1 and vzero are vector char constants instead, the expected behavior is maintained (see attached test2.c rewritten from Guy's test.c).

To understand the difference, the vector int version of vec1 looks like this in a hardware register:

    BE:  00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff    
    LE:  cc dd ee ff 88 99 aa bb 44 55 66 77 00 11 22 33

But the vector char version looks like this:

    BE:  00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff
    LE:  ff ee dd cc bb aa 99 88 77 66 55 44 33 22 11 00

In the latter case, the compiler can generate correct LE code by a simple modification of the BE permute control vector ("vperm" in Guy's test.c) and a reordering of the two input vectors.  That is what all of the compilers do today.  It is not so simple to deal with an arbitrary reordering of bytes within vector elements.

I infer that the goal in test.c is to swap the second and fourth channels of a pixel representation, but instead we are swapping the first and third channels.  It is not so clear to me what the goal of "swizzle it" in ConvertAltivec32to32_prefetch() is without being able to see the "valigner" and "vpermute" vectors, though I would guess it is doing the same thing.  If that is so, then I would recommend making the following source change to the SDL library function:

#if __LITTLE_ENDIAN__
  vpermute = { 2, 1, 0, 3, 2, 1, 0, 3, 2, 1, 0, 3, 2, 1, 0, 3 };
#else
  vpermute = { 0, 3, 2, 1, 0, 3, 2, 1, 0, 3, 2, 1, 0, 3, 2, 1 };
#endif

As shown in the attached test3.c, this will produce the correct result for LE.  (Reason: Numbering from the left for BE, and numbering from the right for LE, produces a different interpretation of what the odd and even lanes are.)

Comment 12 Menanteau Guy 2017-09-06 09:55 UTC
Created attachment 1322598 [details]
correct result with vector char

Comment 13 Menanteau Guy 2017-09-06 09:56 UTC
Created attachment 1322599 [details]
reorder bytes

Comment 14 Menanteau Guy 2017-09-06 10:02 UTC
Created attachment 1322601 [details]
proposed patch

I get the test ok when building pygame-1.9.3-5 with this patch applied on SDL-1.2.15-28.

Comment 15 Sergio Monteiro Basto 2017-09-06 11:39:45 UTC
Hi , I don't know is related but I think it worth to mention [1] , on opencl-heards we also got and issue with  vector , on ppc64le we should use __verctor instead vector ... 


[1] 
https://bugzilla.redhat.com/show_bug.cgi?id=1487174

Comment 16 Sergio Monteiro Basto 2017-09-06 11:45:03 UTC
(fixing typos sorry) 
Hi , I don't know if it is related but I think it worth to mention [1] , on opencl-heards we also got an issue with vector, on ppc64le we should use __verctor instead vector ...

Comment 17 Petr Pisar 2017-09-07 10:16:48 UTC
I still don't understand why this is specific to PowerPC. To me it seems like a bug in PPC64LE GCC's implementation. See this LLVM commit <http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20140602/107034.html> that reorders last argument so that the vec_perm() retains it's platform-specific endianity while it is implemented on top of AlitVec instructions that has big-endian semantics even on PPC64LE.

Shouldn't this be fixed in GCC instead?

Comment 18 Bill Schmidt 2017-09-07 15:46:31 UTC
All the POWER compilers (GCC, LLVM, XLC) have the same implementation of vec_perm for little endian.  There is an implicit assumption that when you specify vec_perm of vector ints, that you want to reorder the vector ints, not reorder the bytes within a vector int.  The SDL code is using each int in a vector int to represent four individual bytes of data, which confuses the compiler's algorithm for converting the permute vector to perform an LE transformation.  This was not anticipated in the design of the vec_perm transformation agreed to by all the compilers, and we will have to do some re-design work in the future to catch these cases.  As this is unlikely to happen quickly, I've recommended a source change to handle this particular unexpected usage.

Comment 19 Bill Schmidt 2017-09-07 16:00:31 UTC
And by the way, to be clear:  I am not fully convinced yet that there even is a transformation that can anticipate this usage in all cases.  When we can determine the permute control vector is a constant, analyzing it is possible.  If it is constructed in another way, there is no way to detect this kind of nonstandard behavior.

Comment 20 Sergio Monteiro Basto 2017-09-15 18:36:35 UTC
Bill , but seems that use __vector instead vector fix the problem [1], I hadn't test in  SDL-1.2  because code have more than 600 vector declarations .


[1]
https://anonscm.debian.org/cgit/pkg-opencl/khronos-opencl-headers.git/tree/debian/patches/use__vector.patch

https://github.com/openembedded/meta-openembedded/commit/97dfbbbf6bddbd360638cde98cb53790ccc83059 

https://github.com/KhronosGroup/OpenCL-Headers/pull/20

Comment 21 Bill Schmidt 2017-09-17 20:25:55 UTC
(In reply to Sergio Monteiro Basto from comment #20)

Replacing vector with __vector cannot fix the problem with usage of vec_perm.

Furthermore, replacing vector with __vector should not be necessary.  This is papering over a bug in the version of Clang that you are using.  You should work with the Clang/LLVM PowerPC community to figure out why that appears to be needed.  This certainly didn't used to be required.

Comment 22 Petr Pisar 2017-09-19 13:53:15 UTC
(In reply to Bill Schmidt from comment #18)
> There is an implicit assumption that when you
> specify vec_perm of vector ints, that you want to reorder the vector ints,
> not reorder the bytes within a vector int.  The SDL code is using each int
> in a vector int to represent four individual bytes of data, which confuses
> the compiler's algorithm for converting the permute vector to perform an LE
> transformation.

Then shouldn't the SDL code be changed to use vector unsigned char instead?

Nevertheless thank you for the patch. I will apply it to the Fedora's SDL. In worst case it will break PPC64LE only that already seems to be broken. We also cannot count with upstream's help because upstream abandoned SDL in favor of SDL2, he does not welcome any bug reports in their Bugzilla instance and they closed mailing list with recommendation to use Discourse web instead.

Comment 23 Fedora Update System 2017-09-19 14:14:45 UTC
SDL-1.2.15-29.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2017-1c27e533b2

Comment 24 Fedora Update System 2017-09-19 14:17:41 UTC
SDL-1.2.15-25.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-f94a70fb4e

Comment 25 Sergio Monteiro Basto 2017-09-19 15:18:40 UTC
(In reply to Bill Schmidt from comment #21)
> (In reply to Sergio Monteiro Basto from comment #20)
> 
> Replacing vector with __vector cannot fix the problem with usage of vec_perm.
> 
> Furthermore, replacing vector with __vector should not be necessary.  This
> is papering over a bug in the version of Clang that you are using.  You
> should work with the Clang/LLVM PowerPC community to figure out why that
> appears to be needed.  This certainly didn't used to be required.

Sorry my mistake/confusion, OK but this is an issue with gcc and I can't let notice another strange error around vector, in openCL when compilation of opencv in ppc64le

/usr/include/CL/cl_platform.h:390:12: error: 'vector' does not name a type; did you mean 'vec_or'?
    typedef vector unsigned char     __cl_uchar16;

what we are missing here , if you can help ? 

[1] 
https://copr-be.cloud.fedoraproject.org/results/sergiomb/opencv/fedora-27-ppc64le/00604161-opencv/build.log.gz (attention all lines are duplicated)

https://copr.fedorainfracloud.org/coprs/sergiomb/opencv/build/604161/

Comment 26 Bill Schmidt 2017-09-19 15:28:41 UTC
What does "/usr/bin/c++ -v" show?  I don't know what version of the compiler you're using.

Comment 27 Bill Schmidt 2017-09-19 15:39:20 UTC
I've double-checked that "typedef vector unsigned char __cl_uchar16;" works fine on recent GCC.  One guess is that the CL code may have done a "#undef vector" at some point; vector is a predefined macro that can be removed in this fashion, in which case only __vector is available.  In this case your workaround would be needed.

This is not uncommon in C++ code where "vector bool" collides with the use of "bool" as a keyword in the language.  Some headers just #undef vector, bool, and pixel to get rid of all the AltiVec predefines.

So all in all I would suggest you go ahead with your workaround.

Sorry for the confusion about Clang; I misunderstood what you were building and how.

Comment 28 Jakub Jelinek 2017-09-19 15:43:33 UTC
Doesn't typedef vector unsigned char __cl_uchar16; only work for -std=gnu++* modes?  For -std=c++* or -ansi it either isn't defined at all, or when altivec.h is included and #undef vector isn't done, then it is totally incompatible with libstdc++ headers.

      if (!flag_iso)
        {
          builtin_define ("vector=vector");
          builtin_define ("pixel=pixel");
          builtin_define ("bool=bool");
          builtin_define ("_Bool=_Bool");
          init_vector_keywords ();

          /* Enable context-sensitive macros.  */
          cpp_get_callbacks (pfile)->macro_to_expand = rs6000_macro_to_expand;
        }

Comment 29 Bill Schmidt 2017-09-19 15:49:08 UTC
(In reply to Jakub Jelinek from comment #28)

Oh, yes.  I had forgotten about this.  If you are building with -ansi, etc., then you will miss these predefines.  However, I don't see a standards directive on the compile command, and I thought -std=gnu++<something> was the default.  I might be wrong about that.  If you add -std=gnu++11 to the command line, does it succeed?

Comment 30 Sergio Monteiro Basto 2017-09-19 15:58:38 UTC
(In reply to Bill Schmidt from comment #26)
> What does "/usr/bin/c++ -v" show?  I don't know what version of the compiler
> you're using.

from build.log.gz :) 
-- The CXX compiler identification is GNU 7.1.1 
-- The C compiler identification is GNU 7.1.1

c++ -v
Using built-in specs.
COLLECT_GCC=/usr/bin/c++
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/7/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --enable-libmpx --enable-offload-targets=nvptx-none --without-cuda-driver --enable-gnu-indirect-function --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 7.1.1 20170802 (Red Hat 7.1.1-7) (GCC) 


(In reply to Bill Schmidt from comment #27)
> I've double-checked that "typedef vector unsigned char __cl_uchar16;" works
> fine on recent GCC.  One guess is that the CL code may have done a "#undef
> vector" at some point; vector is a predefined macro that can be removed in
> this fashion, in which case only __vector is available.  In this case your
> workaround would be needed.
> 
> This is not uncommon in C++ code where "vector bool" collides with the use
> of "bool" as a keyword in the language.  Some headers just #undef vector,
> bool, and pixel to get rid of all the AltiVec predefines.

Hum , maybe I will investigate , I don't have much time now ... 

> So all in all I would suggest you go ahead with your workaround.
> 
> Sorry for the confusion about Clang; I misunderstood what you were building
> and how.

no problem at all , many thanks for your reply .
Cheers. 
(In reply to Bill Schmidt from comment #29)
> (In reply to Jakub Jelinek from comment #28)
> 
> Oh, yes.  I had forgotten about this.  If you are building with -ansi, etc.,
> then you will miss these predefines.  However, I don't see a standards
> directive on the compile command, and I thought -std=gnu++<something> was
> the default.  I might be wrong about that.  If you add -std=gnu++11 to the
> command line, does it succeed?

I will try now

Comment 31 Fedora Update System 2017-09-19 23:30:42 UTC
SDL-1.2.15-29.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-1c27e533b2

Comment 32 Fedora Update System 2017-09-20 00:24:01 UTC
SDL-1.2.15-25.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-f94a70fb4e

Comment 33 Sergio Monteiro Basto 2017-09-22 23:46:13 UTC
(In reply to Bill Schmidt from comment #29)
> (In reply to Jakub Jelinek from comment #28)
> 
> Oh, yes.  I had forgotten about this.  If you are building with -ansi, etc.,
> then you will miss these predefines.  However, I don't see a standards
> directive on the compile command, and I thought -std=gnu++<something> was
> the default.  I might be wrong about that.  If you add -std=gnu++11 to the
> command line, does it succeed?

same thing [1]

/usr/include/CL/cl_platform.h:390:12: error: 'vector' does not name a type; did you mean 'vec_or'?
    typedef vector unsigned char     __cl_uchar16;

/usr/include/CL/cl_platform.h:391:12: error: 'vector' does not name a type; did you mean 'vec_or'?
    typedef vector signed char       __cl_char16;

/usr/include/CL/cl_platform.h:392:12: error: 'vector' does not name a type; did you mean 'vec_or'?
    typedef vector unsigned short    __cl_ushort8;

etc 

[1]
https://copr-be.cloud.fedoraproject.org/results/sergiomb/opencv/fedora-26-ppc64le/00606906-opencv/build.log.gz

https://copr-be.cloud.fedoraproject.org/results/sergiomb/opencv/fedora-26-ppc64le/00606906-opencv/

https://copr.fedorainfracloud.org/coprs/sergiomb/opencv/build/606906/

Comment 34 Fedora Update System 2017-09-26 23:55:23 UTC
SDL-1.2.15-25.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

Comment 35 Fedora Update System 2017-09-30 06:42:43 UTC
SDL-1.2.15-29.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.

Comment 36 Sergio Monteiro Basto 2017-09-30 19:20 UTC
Created attachment 1332750 [details]
0001-Re-enable-tests-on-ppc64le.patch

Built successfully after re-enable tests on ppc64le. 

https://koji.fedoraproject.org/koji/taskinfo?taskID=22166379

Comment 37 Sergio Monteiro Basto 2017-12-02 00:51:37 UTC
tests fail on ppc64le because vec_perm() is fixed closing the bug


Note You need to log in before you can comment on or make changes to this bug.