Bug 86194 - [EFL] Rounding errors on 32-bit machines causes tests to fail
Summary: [EFL] Rounding errors on 32-bit machines causes tests to fail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 87000
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-11 03:58 PDT by Chris Dumez
Modified: 2012-05-21 03:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.71 KB, patch)
2012-05-11 04:02 PDT, Chris Dumez
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch (1.49 KB, patch)
2012-05-11 06:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (1.59 KB, patch)
2012-05-11 07:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.31 KB, patch)
2012-05-13 23:35 PDT, Chris Dumez
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch (2.27 KB, patch)
2012-05-13 23:50 PDT, Chris Dumez
mrobinson: review-
Details | Formatted Diff | Diff
Patch (3.30 KB, patch)
2012-05-15 00:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2012-05-16 00:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-05-11 03:58:35 PDT
Rounding errors on 32-bit machines causes tests to fail.

Usually, the results are 1 pixel off compared to exectations.
Comment 1 Chris Dumez 2012-05-11 04:02:40 PDT
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 2 Gyuyoung Kim 2012-05-11 05:40:51 PDT
Comment on attachment 141376 [details]
Patch

Attachment 141376 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12663667
Comment 3 Chris Dumez 2012-05-11 06:30:51 PDT
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 4 Raphael Kubo da Costa (:rakuco) 2012-05-11 07:11:15 PDT
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.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-05-11 07:12:47 PDT
(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.
Comment 6 Chris Dumez 2012-05-11 07:16:13 PDT
(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.
Comment 7 Chris Dumez 2012-05-11 07:23:11 PDT
Created attachment 141413 [details]
Patch

Take rakuco's feedback into consideration.
Comment 8 Martin Robinson 2012-05-11 08:37:00 PDT
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.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-05-11 13:13:50 PDT
(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?
Comment 10 Chris Dumez 2012-05-13 23:35:26 PDT
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 11 Gyuyoung Kim 2012-05-13 23:45:28 PDT
Comment on attachment 141648 [details]
Patch

Attachment 141648 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12685242
Comment 12 Chris Dumez 2012-05-13 23:50:42 PDT
Created attachment 141651 [details]
Patch

Forgot to remove debug in previous patch.
Comment 13 Martin Robinson 2012-05-14 08:12:45 PDT
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.
Comment 14 Chris Dumez 2012-05-15 00:45:45 PDT
Created attachment 141879 [details]
Patch

Take feedback into consideration.
Comment 15 Martin Robinson 2012-05-15 07:45:18 PDT
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 16 Raphael Kubo da Costa (:rakuco) 2012-05-15 13:32:55 PDT
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.
Comment 17 Chris Dumez 2012-05-16 00:20:18 PDT
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 18 Raphael Kubo da Costa (:rakuco) 2012-05-16 08:17:26 PDT
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?
Comment 19 Raphael Kubo da Costa (:rakuco) 2012-05-16 08:18:31 PDT
I mean, in CMakeLists.txt we also check for mips, and keeping the checks here and there in sync is error-prone.
Comment 20 Martin Robinson 2012-05-16 08:59:30 PDT
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 21 WebKit Review Bot 2012-05-16 09:08:58 PDT
Comment on attachment 142174 [details]
Patch

Clearing flags on attachment: 142174

Committed r117298: <http://trac.webkit.org/changeset/117298>
Comment 22 WebKit Review Bot 2012-05-16 09:09:07 PDT
All reviewed patches have been landed.  Closing bug.