Bug fixes for PowerPC targets #35988

jhamby · 2020-11-06T03:24:33Z

Fixes for Node.js (V8) for older PowerPC targets. I'm testing with
Gentoo Linux on ppc64 (big-endian) on a PowerMac Quad G5, so this
code needs testing on other systems, especially POWER9, which I believe
should have some CPU flags enabled that aren't currently being enabled.
Currently, POWER9 is setting MODULO but not the other feature flags, so
I refactored the code to a switch statement with fallthrough assuming that
the newer POWER CPUs have all the feature flags of the earlier ones.
Please let me know if that assumption is incorrect. Other changes:

PPC64 big-endian ELFv1 ABI uses function descriptors, like AIX.
Tell the code generator when we don't have the FP to int rounding
instructions (added in Power ISA v2.03) by not enabling them in the
MachineOperatorBuilder::Flags if we don't have a new enough CPU.
My first patch tried to emit the equivalent rounding behavior inline,
but then I realized I could set the flags to not emit frim/frin/friz/frip
if the CPU doesn't support those instructions.
Change minimum page size to 4KB for PPC. 64KB physical pages are a
newer feature that breaks some software, such as the nouveau driver.
Change PPC CPU detection to use getauxval() for glibc 2.16 and newer,
and to correctly recognize all known Linux PowerPC platform types.
Cell BE is identified as PPC_G5; other CPUs with AltiVec as PPC_G4.
The new PPC_G3 type is used for all other CPUs without VMX/AltiVec.
Add VMX feature for future VMX/AltiVec code generation. VMX is a
subset of VSX that's available on POWER6, G5, Cell, G4, and PA6T.
The newer VSX feature is only available on POWER7 and newer CPUs.

I need to continue testing, as I'm seeing one strange failure in cctest now,
only with a release build. But I think the patches I have so far are basically
correct and ready for review and feedback. Thanks!

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

* Change minimum page size to 4KB for PPC. 64KB physical pages are a newer feature that breaks some software, such as the nouveau driver. * Change PPC CPU detection to use getauxval() for glibc 2.16 and newer, and to correctly recognize all known Linux PowerPC platform types. Cell BE is identified as PPC_G5; other CPUs with AltiVec as PPC_G4. The new PPC_G3 type is used for all other CPUs without VMX/AltiVec. * Add VMX feature for future VMX/AltiVec code generation. VMX is a subset of VSX that's available on POWER6, G5, Cell, G4, and PA6T. The newer VSX feature is only available on POWER7 and newer CPUs. * Add codegen support for older CPUs that lack the FP to integer rounding instructions added in Power ISA v2.03 (e.g. POWER5+, PA6T). Renamed the feature from FPU to the more descriptive FP_ROUND_TO_INT. Original patch written by A. Wilcox <[email protected]>. Updated to handle -0.0 and NaN cases, and it should work on PPC32.

* PPC64 big-endian ELFv1 ABI uses function descriptors, like AIX. * Tell the code generator when we don't have the FP to int rounding instructions (added in Power ISA v2.03) by not enabling them in the MachineOperatorBuilder::Flags if we don't have a new enough CPU. * Revert the previous patch to generate a complicated branching code sequence to substitute for the FP to int rounding instructions not present on POWER5 and older CPUs. We will no longer call the code to emit these instructions if the feature isn't detected at runtime. * In the code that sets MachineOperatorBuilder::Flags, modify to only set kWord64Popcnt if we're compiling for PPC64. We always set kWord32Popcnt. This may not matter in practice for 32-bit builds.

mscdex · 2020-11-06T06:45:03Z

Have these changes been submitted upstream to V8?

targos · 2020-11-06T08:03:00Z

@nodejs/platform-ppc

john-yan · 2020-11-06T14:26:14Z

Hello @jhamby, thank for your hard work for supporting G5. Do you have plans to continuous supporting this arch in the near term or longer term? If you don't, your work is likely to falling apart very soon when we move foreward, because we don't have HW for testing. Also, v8 changes need to be made on v8 upstream first before floating to node.js. Please follow the Google guildlines to contribute this to V8: here

miladfarca · 2020-11-06T20:48:02Z

deps/v8/src/base/build_config.h

-// PPC has large (64KB) physical pages.
-const int kPageSizeBits = 19;
-#else
+// Use 4KB for all targets. Not all PPC Linux kernels use 64KB pages.
 const int kPageSizeBits = 18;


On newer versions of P Linux, page size needs to remain as 64KB as it's the default size of the OS.
AIX and older Linux versions use 4KB by default. This is causing some V8 test failures on P Linux.

modern Linux allows both 4k and 64k page sizes (and there are modern distros for modern POWER processors that do use 4k pages) but the constant name is misleading - using 64k there will work on both 4k and 64k hosts, while using 4k there will break 64k hosts

miladfarca · 2020-11-06T20:54:21Z

deps/v8/src/heap/base/asm/ppc/push_registers_asm.cc

@@ -33,7 +33,7 @@ asm(
    // At anytime, SP (r1) needs to be multiple of 16 (i.e. 16-aligned).
    "  mflr 0                                          \n"
    "  std 0, 16(1)                                    \n"
-#if defined(_AIX)
+#if ABI_USES_FUNCTION_DESCRIPTORS


ABI_USES_FUNCTION_DESCRIPTORS is not defined in this file, no V8 headers are included here, it's raw assembly, hence defined(_AIX) is used to rely on gcc.

could use defined(__powerpc64__) && (!defined(_CALL_ELF) || (_CALL_ELF == 1)) for a generic check

miladfarca · 2020-11-06T20:55:04Z

deps/v8/src/common/globals.h

@@ -203,7 +203,7 @@ constexpr bool kPlatformRequiresCodeRange = true;
 #if (V8_HOST_ARCH_PPC || V8_HOST_ARCH_PPC64) && \
    (V8_TARGET_ARCH_PPC || V8_TARGET_ARCH_PPC64) && V8_OS_LINUX
 constexpr size_t kMaximalCodeRangeSize = 512 * MB;
-constexpr size_t kMinExpectedOSPageSize = 64 * KB;  // OS page on PPC Linux
+constexpr size_t kMinExpectedOSPageSize = 4 * KB;  // min OS page size


Same comment regarding P linux page size applies here.

q66 · 2020-11-07T05:39:18Z

the pagesize changes shouldn't be included; 64k-assuming code works fine on 4k page hosts (it's just a little bit wasteful, but the only fix for that is checking pagesize at runtime and that would need way bigger changes), but it does not apply the other way around - this is because 64k is a multiple of 4k

also, what's the point of using getauxval there - parsing /proc is effectively equivalent, and this assumption is also incorrect, since getauxval is not a glibc-only feature

q66 · 2020-11-07T05:49:33Z

deps/v8/src/base/cpu.cc

@@ -676,8 +676,14 @@ CPU::CPU()

 #ifndef USE_SIMULATOR
 #if V8_OS_LINUX
+#if V8_GLIBC_PREREQ(2, 16)


getauxval is available on musl and other libc's, so this check is wrong, and there is no reasonable way to check it - i just wouldn't bother, the other path is directly equivalent already

q66 · 2020-11-07T05:58:31Z

also, i believe v8 is currently generally broken on 32-bit PowerPC and will need much more extensive changes in the codegen to fix that; so anything 32-bit relevant here is partial, and it's mostly just fixes for older 64-bit CPUs

awilfox · 2020-11-14T13:16:31Z

Hello @jhamby, thank for your hard work for supporting G5. Do you have plans to continuous supporting this arch in the near term or longer term? If you don't, your work is likely to falling apart very soon when we move foreward, because we don't have HW for testing. Also, v8 changes need to be made on v8 upstream first before floating to node.js. Please follow the Google guildlines to contribute this to V8: here

We at Adélie Linux would be more than happy to maintain this support in perpetuity. Supporting G5/P5/P6 is a core focus of ours. We will be using it every time we bump Node and/or Firefox (since Firefox uses Node in the build).

jhamby added 2 commits November 5, 2020 17:27

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Nov 6, 2020

jhamby mentioned this pull request Nov 6, 2020

Illegal instruction with a cross-compiled node.js binary on a Freescale Qoriq CPU ibmruntimes/node#30

Open

miladfarca reviewed Nov 6, 2020

View reviewed changes

q66 reviewed Nov 7, 2020

View reviewed changes

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixes for PowerPC targets #35988

Bug fixes for PowerPC targets #35988

jhamby commented Nov 6, 2020

mscdex commented Nov 6, 2020

targos commented Nov 6, 2020

john-yan commented Nov 6, 2020

miladfarca Nov 6, 2020

q66 Nov 7, 2020 •

edited

Loading

miladfarca Nov 6, 2020

q66 Nov 7, 2020 •

edited

Loading

miladfarca Nov 6, 2020

q66 commented Nov 7, 2020 •

edited

Loading

q66 Nov 7, 2020

q66 commented Nov 7, 2020

awilfox commented Nov 14, 2020

Bug fixes for PowerPC targets #35988

Bug fixes for PowerPC targets #35988

Conversation

jhamby commented Nov 6, 2020

Checklist

mscdex commented Nov 6, 2020

targos commented Nov 6, 2020

john-yan commented Nov 6, 2020

miladfarca Nov 6, 2020

Choose a reason for hiding this comment

q66 Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

miladfarca Nov 6, 2020

Choose a reason for hiding this comment

q66 Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

miladfarca Nov 6, 2020

Choose a reason for hiding this comment

q66 commented Nov 7, 2020 • edited Loading

q66 Nov 7, 2020

Choose a reason for hiding this comment

q66 commented Nov 7, 2020

awilfox commented Nov 14, 2020

q66 Nov 7, 2020 •

edited

Loading

q66 Nov 7, 2020 •

edited

Loading

q66 commented Nov 7, 2020 •

edited

Loading