Rounding errors on 32-bit machines causes tests to fail. Usually, the results are 1 pixel off compared to exectations.
Created attachment 141376 [details] Patch Use scalar floating-point instructions present in the SSE instruction set on 32-bit. This is the default choice for the x86-64 compiler but not for 32-bit. Without enabling this, we get a lot of rounding errors on 32-bit machines causing a lot of tests to fail (off by 1 pixel). The same issue was reported on GTK port and solved in the same way: Bug 72254
Comment on attachment 141376 [details] Patch Attachment 141376 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12663667
Created attachment 141399 [details] Patch Fix patch so that the flags are used only on 32 bit. The patch is now almost the same as for GTK.
Comment on attachment 141399 [details] Patch webkitdirs.pm looks like the wrong place to do this (and determineArchitecture() only seems to work for GTK and Mac). We already detect the processor type in the top-level CMakeLists.txt and define some compiler flags in Source/cmake/WebKitHelpers.cmake, so I guess it makes sense to add these to the latter.
(In reply to comment #4) > (From update of attachment 141399 [details]) > webkitdirs.pm looks like the wrong place to do this (and determineArchitecture() only seems to work for GTK and Mac). We already detect the processor type in the top-level CMakeLists.txt and define some compiler flags in Source/cmake/WebKitHelpers.cmake, so I guess it makes sense to add these to the latter. Hmm, I've just seen your first attempt there. If you enclose your SET() call within an IF (WTF_CPU_X86) it should work.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 141399 [details] [details]) > > webkitdirs.pm looks like the wrong place to do this (and determineArchitecture() only seems to work for GTK and Mac). We already detect the processor type in the top-level CMakeLists.txt and define some compiler flags in Source/cmake/WebKitHelpers.cmake, so I guess it makes sense to add these to the latter. > > Hmm, I've just seen your first attempt there. If you enclose your SET() call within an IF (WTF_CPU_X86) it should work. Thanks, I'll do that.
Created attachment 141413 [details] Patch Take rakuco's feedback into consideration.
The reason we did this in webkitdirs.pm is that -march=pentium4 -msse2 -mfpmath=sse could have some unknown performance impact. This is acceptable on our bots, which just test correctness, but not acceptable to ship in our releases and to downstream.
(In reply to comment #8) > The reason we did this in webkitdirs.pm is that -march=pentium4 -msse2 -mfpmath=sse could have some unknown performance impact. This is acceptable on our bots, which just test correctness, but not acceptable to ship in our releases and to downstream. That makes sense, with my FreeBSD packager hat on I find it bad when upstream hardcodes compiler flags that way. In that case, shouldn't users just export those compiler flags locally then?
Created attachment 141648 [details] Patch Take feedback into consideration. Now, setting the flags in Tools/Scripts/webkitdirs.pm and make determineArchitecture() function work on EFL port.
Comment on attachment 141648 [details] Patch Attachment 141648 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12685242
Created attachment 141651 [details] Patch Forgot to remove debug in previous patch.
Comment on attachment 141651 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141651&action=review Seems sane, though I think this could use a little cleanup. > Tools/Scripts/webkitdirs.pm:333 > + } else { > + # Fall back to output of `arch', if it is present. > + $architecture = `arch`; > + chomp $architecture; The fallback is now repeated three times. Perhaps it's better to put it in a helper function at this point or restructure the code so it's not repeated. > Tools/Scripts/webkitdirs.pm:2100 > + if ($architecture ne "x86_64" && !isARM()) { > + $ENV{'CXXFLAGS'} = "-march=pentium4 -msse2 -mfpmath=sse"; > + } You're completely overwriting CXXFLAGS here. It might be better to preserve any that already exist.
Created attachment 141879 [details] Patch Take feedback into consideration.
Comment on attachment 141879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141879&action=review Looks good, though I have one doubt. > Tools/Scripts/webkitdirs.pm:328 > + if (!$architecture) { I'm a little bit worried here because it looks almost like this will call arch on all platforms now. I'm not sure if calling arch for !isGtk && !isAppleMacWebKit && !isEfl is a problem here.
Comment on attachment 141879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141879&action=review I still wonder if it doesn't make sense to simply check for the presence of these flags at configure-time and warn the user depending on the processor and/or document it somewhere? > Tools/Scripts/webkitdirs.pm:320 > + my $cmakesystem_file = "$configurationProductDir/CMakeFiles/CMakeSystem.cmake"; This requires that one to have run CMake for this file to exist. Capturing the output of `cmake --system-information' is better. > Tools/Scripts/webkitdirs.pm:324 > + $architecture = $1; This has the same problem we have solved in the top-level CMakeLists.txt -- "x86_64" might be returned as "amd64" on some platforms.
Created attachment 142174 [details] Patch Take feedback into consideration. I still prefer adding the flags automatically in the build script like other ports do. I know the user could do it manually but it is less convenient.
Comment on attachment 142174 [details] Patch I guess that should work; one last thing: why not check if the architecture is x86 instead of checking if it's not something else?
I mean, in CMakeLists.txt we also check for mips, and keeping the checks here and there in sync is error-prone.
Comment on attachment 142174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142174&action=review > Tools/Scripts/webkitdirs.pm:1872 > + $ENV{'CXXFLAGS'} = "-march=pentium4 -msse2 -mfpmath=sse " . $ENV{'CXXFLAGS'}; Thanks for fixing this one as well.
Comment on attachment 142174 [details] Patch Clearing flags on attachment: 142174 Committed r117298: <http://trac.webkit.org/changeset/117298>
All reviewed patches have been landed. Closing bug.