Bug 208731

Summary: Fix some issues in the ARM64 moveConditionallyAfterFloatingPointCompare() and moveDoubleConditionallyAfterFloatingPointCompare().
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer, fpizlo, iseenoface, justin_michaud, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=208736
https://bugs.webkit.org/show_bug.cgi?id=208738
Attachments:
Description Flags
proposed patch. saam: review+

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>