Bug 208731 - Fix some issues in the ARM64 moveConditionallyAfterFloatingPointCompare() and moveDoubleConditionallyAfterFloatingPointCompare().
Summary: Fix some issues in the ARM64 moveConditionallyAfterFloatingPointCompare() and...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-06 12:43 PST by Mark Lam
Modified: 2020-03-06 21:54 PST (History)
10 users (show)

See Also:


Attachments
proposed patch. (50.58 KB, patch)
2020-03-06 13:08 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-03-06 12:43:56 PST
Details to follow in the ChangeLog.

<rdar://problem/59222568>
Comment 1 Mark Lam 2020-03-06 13:08:02 PST
Created attachment 392762 [details]
proposed patch.
Comment 2 Saam Barati 2020-03-06 14:05:42 PST
Comment on attachment 392762 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=392762&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2030
>          if (cond == DoubleNotEqual) {

can you file a bug that DoubleNotEqual really means DoubleNotEqualAndNotUnordered?
Comment 3 Saam Barati 2020-03-06 14:19:17 PST
Comment on attachment 392762 [details]
proposed patch.

there is more to be done here. We're calling it "cmov", but we're now implementing it as select. Is that ok?
Comment 4 Saam Barati 2020-03-06 14:30:33 PST
Comment on attachment 392762 [details]
proposed patch.

Since these moveConditionally are really select, maybe we should rename it?
Comment 5 Mark Lam 2020-03-06 14:51:29 PST
Thanks for the review.

Filed the following for the naming issues:
1. https://bugs.webkit.org/show_bug.cgi?id=208736 Add "AndOrdered" to the names of ordered DoubleConditions.
2. https://bugs.webkit.org/show_bug.cgi?id=208738 Consider renaming moveConditionally... MacroAssembler emitters.
Comment 6 Mark Lam 2020-03-06 17:17:43 PST
Landed in r258038: <http://trac.webkit.org/r258038>.
Comment 7 David Kilzer (:ddkilzer) 2020-03-06 21:54:04 PST
(In reply to Mark Lam from comment #6)
> Landed in r258038: <http://trac.webkit.org/r258038>.

This caused a build failure on Windows 10 build bots:

C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(532): error C2220: the following warning is treated as an error [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(532): warning C4715: '``anonymous namespace'::testCompareDouble'::`2'::<lambda_3>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(603): warning C4715: '``anonymous namespace'::testCompareDoubleSameArg'::`2'::<lambda_3>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(727): warning C4715: '``anonymous namespace'::testMoveConditionallyFloatingPoint<double,unsigned int>'::`2'::<lambda_1>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(727): warning C4715: '``anonymous namespace'::testMoveConditionallyFloatingPoint<float,unsigned int>'::`2'::<lambda_1>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(727): warning C4715: '``anonymous namespace'::testMoveConditionallyFloatingPoint<double,double>'::`2'::<lambda_1>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(1299): warning C4715: '``anonymous namespace'::testMoveConditionallyFloatingPointSameArg<double,unsigned int>'::`2'::<lambda_1>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(727): warning C4715: '``anonymous namespace'::testMoveConditionallyFloatingPoint<float,double>'::`2'::<lambda_1>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(1299): warning C4715: '``anonymous namespace'::testMoveConditionallyFloatingPointSameArg<float,unsigned int>'::`2'::<lambda_1>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(1299): warning C4715: '``anonymous namespace'::testMoveConditionallyFloatingPointSameArg<float,double>'::`2'::<lambda_1>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]
C:\cygwin\home\buildbot\worker\win10-debug\build\Source\JavaScriptCore\assembler\testmasm.cpp(1299): warning C4715: '``anonymous namespace'::testMoveConditionallyFloatingPointSameArg<double,double>'::`2'::<lambda_1>::operator()': not all control paths return a value [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\shell\testmasmLib.vcxproj]

<https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/builds/11014/steps/compile-webkit/logs/stdio>

Committed r258062: <http://trac.webkit.org/r258062>