Bug 86194 – [EFL] Rounding errors on 32-bit machines causes tests to fail

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

Rounding errors on 32-bit machines causes tests to fail.

Usually, the results are 1 pixel off compared to exectations.

[Chris Dumez]

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

[Gyuyoung Kim]

Comment on attachment 141376 [details]
Patch

Attachment 141376 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12663667

[Chris Dumez]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 141413 [details]
Patch

Take rakuco's feedback into consideration.

[Martin Robinson]

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.

[Raphael Kubo da Costa (:rakuco)]

(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?

[Chris Dumez]

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.

[Gyuyoung Kim]

Comment on attachment 141648 [details]
Patch

Attachment 141648 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12685242

[Chris Dumez]

Created attachment 141651 [details]
Patch

Forgot to remove debug in previous patch.

[Martin Robinson]

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.

[Chris Dumez]

Created attachment 141879 [details]
Patch

Take feedback into consideration.

[Martin Robinson]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Chris Dumez]

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.

[Raphael Kubo da Costa (:rakuco)]

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?

[Raphael Kubo da Costa (:rakuco)]

I mean, in CMakeLists.txt we also check for mips, and keeping the checks here and there in sync is error-prone.

[Martin Robinson]

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.

[WebKit Review Bot]

Comment on attachment 142174 [details]
Patch

Clearing flags on attachment: 142174

Committed r117298: <http://trac.webkit.org/changeset/117298>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.