Bug 175514 – [GTK] ARMv7 build fails to build MacroAssemblerARMv7.cpp.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

See. https://bugs.webkit.org/show_bug.cgi?id=175446#c24:

/tmp/cco6SOuf.s: Assembler messages:
/tmp/cco6SOuf.s:48: Error: VFP single precision register expected -- `vstmia.64 ip!,{ d16-d31 }'
/tmp/cco6SOuf.s:55: Error: VFP single precision register expected -- `vldmdb.64 ip!,{ d16-d31 }'
/tmp/cco6SOuf.s:88: writing to APSR without specifying a bitmask is deprecated

[Mark Lam]

@Ossy, I looked into this as much as I can from OS(DARWIN) side and cannot find what the issue is.

> /tmp/cco6SOuf.s:48: Error: VFP single precision register expected --
> `vstmia.64 ip!,{ d16-d31 }'
> /tmp/cco6SOuf.s:55: Error: VFP single precision register expected --
> `vldmdb.64 ip!,{ d16-d31 }'

The .64 is supposed to tell the assembler that these instruction operate on the double precision registers.

> /tmp/cco6SOuf.s:88: writing to APSR without specifying a bitmask is
> deprecated

I cannot find any ARM documentation that says the MSR instruction can take an extra bitmask.  The closest possibility is that APSR needs to be specified as APSR_nzcvq.  However, Clang does not like using APSR_nzcvq.

In the end, both of these issue appears to be due to the tool chain on GTK's side.  Can someone on the GTK side investigate this?

The alternative would be to disable the DFG for non OS(DARWIN) builds.

[Csaba Osztrogonác]

no idea what problem GTK has ... cc-ed GTK guys

Now JSCOnly builds are happy, this one is the only one remaining issue.

[Carlos Alberto Lopez Perez]

(In reply to Mark Lam from comment #1)
> @Ossy, I looked into this as much as I can from OS(DARWIN) side and cannot
> find what the issue is.
> 
> > /tmp/cco6SOuf.s:48: Error: VFP single precision register expected --
> > `vstmia.64 ip!,{ d16-d31 }'
> > /tmp/cco6SOuf.s:55: Error: VFP single precision register expected --
> > `vldmdb.64 ip!,{ d16-d31 }'
> 
> The .64 is supposed to tell the assembler that these instruction operate on
> the double precision registers.
> 
> > /tmp/cco6SOuf.s:88: writing to APSR without specifying a bitmask is
> > deprecated
> 
> I cannot find any ARM documentation that says the MSR instruction can take
> an extra bitmask.  The closest possibility is that APSR needs to be
> specified as APSR_nzcvq.  However, Clang does not like using APSR_nzcvq.
> 
> In the end, both of these issue appears to be due to the tool chain on GTK's
> side.  Can someone on the GTK side investigate this?
> 
> The alternative would be to disable the DFG for non OS(DARWIN) builds.

GTK+ ARM bots use Thumb2 instructions.

[Carlos Alberto Lopez Perez]

This may be related to the FPU the compiler assume it can use. More info: https://stackoverflow.com/a/33904219

Kov, can you please paste here the output of running "gcc -v" on the ARM buildbot (to see which "--with-fpu" parameter it was built with)?

[Mark Lam]

To green the GTX bots, I've disabled the DFG for GTK ARM_THUMB2 in r220816: <http://trac.webkit.org/r220816>.

Please re-enabled the DFG when this issue is fixed.

[Carlos Alberto Lopez Perez]

(In reply to Mark Lam from comment #5)
> To green the GTX bots, I've disabled the DFG for GTK ARM_THUMB2 in r220816:
> <http://trac.webkit.org/r220816>.
> 
> Please re-enabled the DFG when this issue is fixed.

It looks is still broken?

(In reply to Carlos Alberto Lopez Perez from comment #4)
> This may be related to the FPU the compiler assume it can use. More info:
> https://stackoverflow.com/a/33904219
> 
> Kov, can you please paste here the output of running "gcc -v" on the ARM
> buildbot (to see which "--with-fpu" parameter it was built with)?

This is the output of gcc -v on Debian armhf (The GTK+ ARM bot runs on Debian)

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/arm-linux-gnueabihf/6/lto-wrapper
Target: arm-linux-gnueabihf
Configured with: ../src/configure -v --with-pkgversion='Debian 6.3.0-18' --with-bugurl=file:///usr/share/doc/gcc-6/README.Bugs --enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-6 --program-prefix=arm-linux-gnueabihf- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libitm --disable-libquadmath --enable-plugin --enable-default-pie --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-6-armhf/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-6-armhf --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-6-armhf --with-arch-directory=arm --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --disable-sjlj-exceptions --with-arch=armv7-a --with-fpu=vfpv3-d16 --with-float=hard --with-mode=thumb --enable-checking=release --build=arm-linux-gnueabihf --host=arm-linux-gnueabihf --target=arm-linux-gnueabihf
Thread model: posix
gcc version 6.3.0 20170516 (Debian 6.3.0-18) 


So, by default its building with --with-fpu=vfpv3-d16

Which according to the GCC help <https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html> means:

‘+vfpv3-d16’

    The VFPv3 floating-point instructions, with 16 double-precision registers.

And the ARM help says this: <http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473c/Bcfhdhgd.html>

So, my understanding is that when building with --with-fpu=vfpv3-d16 only the double-precision registers d0-d15 are available.

To access the double-precision registers beyond d15 (d16-d31) we need to build with vfpv4/neon or something like -mfpu=vfpv3-fp16


I'm unsure about the consequences of requiring a vfpu with 32 double-precision registers. Do all ARMv7 CPUs support this???

[Mark Lam]

(In reply to Carlos Alberto Lopez Perez from comment #6)
> (In reply to Mark Lam from comment #5)
> > To green the GTX bots, I've disabled the DFG for GTK ARM_THUMB2 in r220816:
> > <http://trac.webkit.org/r220816>.
> > 
> > Please re-enabled the DFG when this issue is fixed.
> 
> It looks is still broken?

https://bugs.webkit.org/show_bug.cgi?id=175656 should take care of it.

[Csaba Osztrogonác]

vfpv3-d16? Do you really have a SoC that doesn't support vfpv4? It means it doesn't support NEON. Nowadays it's hard 
to find this kind of SoC.

I can imagine that you use the default toolchain of the OS,
which is too conservative and try to support very old
CPUs too by default. I think you should check if your
SoC has vfpv4. If yes, you should just exploit it. If no,
it is a good question .... Mark, does the ARMv7 Thumb2 backend rely on 32 VFP registers?

[Csaba Osztrogonác]

It seems that only Tegra2 and Marvell dove SoCs don't have 32 VFP registers, but only 16, because they don't have NEON support. Are you really want to support these SoCs?

[Carlos Alberto Lopez Perez]

(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #9)
> It seems that only Tegra2 and Marvell dove SoCs don't have 32 VFP registers,
> but only 16, because they don't have NEON support. Are you really want to
> support these SoCs?

Thanks for the info. I had no idea which kind of SoCs where those. This look pretty old indeed.

vfpv3-d16 is the default FPU setting in the Debian toolchain. I think Marvell is one of the SoCs supported by Debian.


https://wiki.debian.org/ArmHardFloatPort#VFP
https://wiki.debian.org/InstallingDebianOn/Marvell

Likely the machine where the GTK+ bot runs supports something better than that. The thing is that Debian defaults to --with-fpu=vfpv3-d16 and we are using the compiler defaults.

I guess the best thing we can do here is to detect on CMake if the compiler supports a FPU with 32 VFP registers and pass the right -fpu flag. If it doesn't then disable MASM (and JIT).

WDYT?

[Mark Lam]

Created attachment 318363 [details]
proposed patch.

Please try this patch to see if it resolves the GTK build issue.  It addresses the absence of NEON, but doesn't do anything about the ASPR complaint yet.

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #10)

> I guess the best thing we can do here is to detect on CMake if the compiler
> supports a FPU with 32 VFP registers and pass the right -fpu flag. If it
> doesn't then disable MASM (and JIT).
> 
> WDYT?

Then we can end up in the opposite situation: The compiler may support
generating code for FPUs with 32 VFP registers, and the target device
may not have the hardware — which would end up in odd failures, most
likely the CPU trapping on illegal instructions.

What probably would be safer is making CMake try to build a snippet
with the configured compiler+flags which uses double-precision VFP
registers (e.g. “vstmia.64”) and, if that succeeds, then MASM gets
enabled. Then we can set “$CXXFLAGS” and “$CFLAGS” in the environment
of the build bots to pass the “-mfpu=cfpv4” flag (or whichever other
value is appropriate for the bot).

[Mark Lam]

(In reply to Adrian Perez from comment #12)
> (In reply to Carlos Alberto Lopez Perez from comment #10)
> 
> > I guess the best thing we can do here is to detect on CMake if the compiler
> > supports a FPU with 32 VFP registers and pass the right -fpu flag. If it
> > doesn't then disable MASM (and JIT).
> > 
> > WDYT?
> 
> Then we can end up in the opposite situation: The compiler may support
> generating code for FPUs with 32 VFP registers, and the target device
> may not have the hardware — which would end up in odd failures, most
> likely the CPU trapping on illegal instructions.
> 
> What probably would be safer is making CMake try to build a snippet
> with the configured compiler+flags which uses double-precision VFP
> registers (e.g. “vstmia.64”) and, if that succeeds, then MASM gets
> enabled. Then we can set “$CXXFLAGS” and “$CFLAGS” in the environment
> of the build bots to pass the “-mfpu=cfpv4” flag (or whichever other
> value is appropriate for the bot).

According to Platform.h, we already have such a flag: CPU(ARM_NEON).  Please try the attached patch and let me know if it resolves the GTK build issue.

[Mark Lam]

Comment on attachment 318363 [details]
proposed patch.

Regardless of whether this patch resolves the entire GTK build issue or not, it is correct.  I'm going to track it over in https://bugs.webkit.org/show_bug.cgi?id=175672 and just block the current bug on 175672.

[Carlos Alberto Lopez Perez]

(In reply to Mark Lam from comment #14)
> Comment on attachment 318363 [details]
> proposed patch.
> 
> Regardless of whether this patch resolves the entire GTK build issue or not,
> it is correct.  I'm going to track it over in
> https://bugs.webkit.org/show_bug.cgi?id=175672 and just block the current
> bug on 175672.

So after r220871 <http://trac.webkit.org/r220871> its possible to enable MASM (and the JIT) on ARMv7 CPUs with only 16 double VFP registers??


If that is the case, then should we revert r220823 <http://trac.webkit.org/r220823> and r220816 <http://trac.webkit.org/r220816> ?

[Mark Lam]

(In reply to Carlos Alberto Lopez Perez from comment #15)
> (In reply to Mark Lam from comment #14)
> > Comment on attachment 318363 [details]
> > proposed patch.
> > 
> > Regardless of whether this patch resolves the entire GTK build issue or not,
> > it is correct.  I'm going to track it over in
> > https://bugs.webkit.org/show_bug.cgi?id=175672 and just block the current
> > bug on 175672.
> 
> So after r220871 <http://trac.webkit.org/r220871> its possible to enable
> MASM (and the JIT) on ARMv7 CPUs with only 16 double VFP registers??
> 
> 
> If that is the case, then should we revert r220823
> <http://trac.webkit.org/r220823> and r220816
> <http://trac.webkit.org/r220816> ?

Did you actually enable MASM_PROBE for GTK and actually build for the ARM_THUMB2 target to confirm that it works?  

If so, then you can add GTK back to the configurations that enable DFG_JIT.  Please leave http://trac.webkit.org/r220823 in place.  I think it's useful to keep around for a while.

[Carlos Alberto Lopez Perez]

(In reply to Mark Lam from comment #16)
> (In reply to Carlos Alberto Lopez Perez from comment #15)
> > (In reply to Mark Lam from comment #14)
> > > Comment on attachment 318363 [details]
> > > proposed patch.
> > > 
> > > Regardless of whether this patch resolves the entire GTK build issue or not,
> > > it is correct.  I'm going to track it over in
> > > https://bugs.webkit.org/show_bug.cgi?id=175672 and just block the current
> > > bug on 175672.
> > 
> > So after r220871 <http://trac.webkit.org/r220871> its possible to enable
> > MASM (and the JIT) on ARMv7 CPUs with only 16 double VFP registers??
> > 
> > 
> > If that is the case, then should we revert r220823
> > <http://trac.webkit.org/r220823> and r220816
> > <http://trac.webkit.org/r220816> ?
> 
> Did you actually enable MASM_PROBE for GTK and actually build for the
> ARM_THUMB2 target to confirm that it works?  
> 

It will take me a while to test this, I don't have access to a Debian ARM machine now. I will try to setup a RPi with it this weekend.

> If so, then you can add GTK back to the configurations that enable DFG_JIT. 
> Please leave http://trac.webkit.org/r220823 in place.  I think it's useful
> to keep around for a while.

The issue with r220823 is that building with MASM_PROBE disabled looks broken (at least on GTK). The GTK ARM bot continues to fail to build. See: https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/1050/steps/compile-webkit/logs/stdio/text


Btw.. thanks for the fixes

[Mark Lam]

(In reply to Carlos Alberto Lopez Perez from comment #17)
> The issue with r220823 is that building with MASM_PROBE disabled looks
> broken (at least on GTK). The GTK ARM bot continues to fail to build. See:
> https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/1050/
> steps/compile-webkit/logs/stdio/text

I think this should be fixed as of r220921: <http://trac.webkit.org/r220921>.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #17)
> > Did you actually enable MASM_PROBE for GTK and actually build for the
> > ARM_THUMB2 target to confirm that it works?  
> > 
> 
> It will take me a while to test this, I don't have access to a Debian ARM
> machine now. I will try to setup a RPi with it this weekend.
> 

Tested: It builds fine and JSC tests look ok.

I'm uploading patch with the fix

[Carlos Alberto Lopez Perez]

Created attachment 318701 [details]
Patch

[Keith Miller]

Comment on attachment 318701 [details]
Patch

r=me.

[WebKit Commit Bot]

Comment on attachment 318701 [details]
Patch

Clearing flags on attachment: 318701

Committed r220993: <http://trac.webkit.org/changeset/220993>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34004175>