Re: [PATCH] Native spinlock support on RISC-V
Re: Thomas Munro > I randomly remembered this topic after seeing an s390x announcement > from Christoph[1], and figured he or someone else might be interested > in the same observation about that platform. That is, we finally got > around to defining this for ARM, but I bet one internet point that > RISCV64 and s390x have that property too (but would need to find a > written reference, or perhaps a similar declaration in the Linux > sources, etc): Fwiw, while s390x is admittedly more of historical interest, riscv64 is well on the way of becoming an official release architecture with the next Debian stable release, so having it supported properly by PostgreSQL would be appreciated. Christoph
Re: [PATCH] Native spinlock support on RISC-V
On Wed, Nov 3, 2021 at 11:55 AM Thomas Munro wrote: > 1. Even though we're using generic built-ins for atomics, I guess we > could still use a src/include/port/atomics/arch-riscv.h file so we > have a place to define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY when > building for 64 bit. We'd need to find the chapter-and-verse to > justify that, of course, which I can try to do; if someone more > familiar with the ISA/spec/manual can point to it I'm all ears. I randomly remembered this topic after seeing an s390x announcement from Christoph[1], and figured he or someone else might be interested in the same observation about that platform. That is, we finally got around to defining this for ARM, but I bet one internet point that RISCV64 and s390x have that property too (but would need to find a written reference, or perhaps a similar declaration in the Linux sources, etc): $ git grep '#define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY' src/include/port/atomics/arch-arm.h:#define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY src/include/port/atomics/arch-ppc.h:#define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY src/include/port/atomics/arch-x86.h:#define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY [1] https://www.postgresql.org/about/news/postgresql-on-s390x-on-debian-and-ubuntu-2752/
Re: [PATCH] Native spinlock support on RISC-V
On Wed, Nov 3, 2021 at 5:41 PM Thomas Munro wrote: > 'generic' is not a recognized processor for this target (ignoring processor) I still don't know what's wrong but I spent 20 minutes searching for more clues this morning... First, 'generic' is coming from LLVMGetHostCPUName(), and yet it's not accepted by LLVMCreateTargetMachine(). Ok then, let's try something else: $ clang12 --print-supported-cpus Target: riscv64-portbld-freebsd13.0 Thread model: posix InstalledDir: /usr/local/llvm12/bin Available CPUs for this target: generic-rv32 generic-rv64 rocket-rv32 rocket-rv64 sifive-7-rv32 sifive-7-rv64 sifive-e31 sifive-e76 sifive-u54 sifive-u74 So I hacked my copy of PostgreSQL thusly: + cpu = LLVMCreateMessage("generic-rv64"); ... and now I get: 2021-11-03 20:27:28.487 UTC [26880] FATAL: fatal llvm error: Relocation type not implemented yet! Taking that at face value, instead of LLVMRelocDefault, I tried a couple of other values like LLVMRelocDynamicNoPic, but same result. So I attached a debugger to see the stack producing the error, and huh, I see a clue that it's confusing this architecture with SystemZ (= IBM mainframe). (gdb) break fatal_llvm_error_handler Function "fatal_llvm_error_handler" not defined. Make breakpoint pending on future shared library load? (y or [n]) y Breakpoint 1 (fatal_llvm_error_handler) pending. (gdb) cont Continuing. Breakpoint 1, fatal_llvm_error_handler (user_data=0x0, reason="Relocation type not implemented yet!", gen_crash_diag=true) at llvmjit_error.cpp:147 147ereport(FATAL, (gdb) bt #0 fatal_llvm_error_handler (user_data=0x0, reason="Relocation type not implemented yet!", gen_crash_diag=true) at llvmjit_error.cpp:147 #1 0x4dc3729a in llvm::report_fatal_error(llvm::Twine const&, bool) () from /usr/local/llvm12/lib/libLLVM-12.so #2 0x4dc371d0 in llvm::report_fatal_error(char const*, bool) () from /usr/local/llvm12/lib/libLLVM-12.so #3 0x4ef61010 in llvm::RuntimeDyldELF::resolveSystemZRelocation(llvm::SectionEntry const&, unsigned long, unsigned long, unsigned int, long) () from /usr/local/llvm12/lib/libLLVM-12.so #4 0x4ef547c6 in llvm::RuntimeDyldImpl::applyExternalSymbolRelocations(llvm::StringMap) () from /usr/local/llvm12/lib/libLLVM-12.so #5 0x4ef54fb4 in ?? () from /usr/local/llvm12/lib/libLLVM-12.so #6 0x4ef54be6 in llvm::RuntimeDyldImpl::finalizeAsync(std::__1::unique_ptr >, llvm::unique_function, std::__1::unique_ptr >, llvm::Error)>, llvm::object::OwningBinary, std::__1::unique_ptr >) () from /usr/local/llvm12/lib/libLLVM-12.so #7 0x4ef55e2a in llvm::jitLinkForORC(llvm::object::OwningBinary, llvm::RuntimeDyld::MemoryManager&, llvm::JITSymbolResolver&, bool, llvm::unique_function, std::__1::allocator > >)>, llvm::unique_function, std::__1::unique_ptr >, llvm::Error)>) () from /usr/local/llvm12/lib/libLLVM-12.so #8 0x4ef421ea in llvm::orc::RTDyldObjectLinkingLayer::emit(std::__1::unique_ptr >, std::__1::unique_ptr >) () from /usr/local/llvm12/lib/libLLVM-12.so #9 0x4ef3e946 in llvm::orc::ObjectTransformLayer::emit(std::__1::unique_ptr >, std::__1::unique_ptr >) () from /usr/local/llvm12/lib/libLLVM-12.so #10 0x4ef25c0c in llvm::orc::IRCompileLayer::emit(std::__1::unique_ptr >, llvm::orc::ThreadSafeModule) () from /usr/local/llvm12/lib/libLLVM-12.so #11 0x4ef25fb2 in llvm::orc::IRTransformLayer::emit(std::__1::unique_ptr >, llvm::orc::ThreadSafeModule) () from /usr/local/llvm12/lib/libLLVM-12.so #12 0x4ef25fb2 in llvm::orc::IRTransformLayer::emit(std::__1::unique_ptr >, llvm::orc::ThreadSafeModule) () from /usr/local/llvm12/lib/libLLVM-12.so #13 0x4ef2a614 in llvm::orc::BasicIRLayerMaterializationUnit::materialize(std::__1::unique_ptr >) () from /usr/local/llvm12/lib/libLLVM-12.so #14 0x4ef08dc2 in ?? () from /usr/local/llvm12/lib/libLLVM-12.so #15 0x4ef0f61c in std::__1::__function::__func >, std::__1::unique_ptr >), std::__1::allocator >, std::__1::unique_ptr >)>, void (std::__1::unique_ptr >, std::__1::unique_ptr >)>::operator()(std::__1::unique_ptr >&&, std::__1::unique_ptr >&&) () from /usr/local/llvm12/lib/libLLVM-12.so #16 0x4ef09c12 in llvm::orc::ExecutionSession::dispatchOutstandingMUs() () from /usr/local/llvm12/lib/libLLVM-12.so #17 0x4ef0b684 in llvm::orc::ExecutionSession::OL_completeLookup(std::__1::unique_ptr >, std::__1::shared_ptr, std::__1::function >, llvm::DenseMapInfo, llvm::detail::DenseMapPair > > > const&)>) () from /usr/local/llvm12/lib/libLLVM-12.so #18 0x4ef141ee in ?? () from /usr/local/llvm12/lib/libLLVM-12.so #19 0x4ef01c1e in llvm::orc::ExecutionSession::OL_applyQueryPhase1(std::__1::unique_ptr >, llvm::Error) () from /usr/local/llvm12/lib/libLLVM-12.so #20 0x4ef0096c in llvm::orc::ExecutionSession::lookup(llvm::orc::LookupKind, std::__1::vector, std::__1::allocator > > const&,
Re: [PATCH] Native spinlock support on RISC-V
On Wed, Nov 3, 2021 at 5:13 PM Andres Freund wrote: > Any chance you could enable jit_dump_bitcode and manually try a failing > query? That should dump. bc files in the data directory. That'd might allow > debugging this outside the emulated environment. > > I don't see where the undef is originating from, but I think it might suggest > that something lead to that code being considered unreachable. postgres=# set jit_above_cost = 0; SET postgres=# select 1 + 1; FATAL: fatal llvm error: Cannot select: 0x4b3ec1a0: i64,ch = load<(load 8 from %ir.14)> 0x41ef6fe8, 0x4b3ec138, undef:i64 0x4b3ec138: i64 = add nuw 0x4b3eab60, Constant:i64<24> 0x4b3eab60: i64,ch = load<(load 8 from %ir.7)> 0x41ef6fe8, 0x4b3eaaf8, undef:i64 0x4b3eaaf8: i64 = add nuw 0x4b3ea068, Constant:i64<16> 0x4b3ea068: i64,ch = CopyFromReg 0x41ef6fe8, Register:i64 %4 0x4b3ea000: i64 = Register %4 0x4b3ea888: i64 = Constant<16> 0x4b3ea6e8: i64 = undef 0x4b3ea9c0: i64 = Constant<24> 0x4b3ea6e8: i64 = undef In function: evalexpr_0_0 Ah, I hadn't noticed this in the log before: 'generic' is not a recognized processor for this target (ignoring processor) Sounds kinda serious :-) Resulting .bc files and .ll files (produced by llvm-dis) attached. 75392.0.bc Description: Binary data 75392.0.optimized.bc Description: Binary data 75392.0.ll Description: Binary data 75392.0.optimized.ll Description: Binary data
Re: [PATCH] Native spinlock support on RISC-V
Hi, On November 2, 2021 3:55:58 PM PDT, Thomas Munro wrote: >2. When configured with all options on FreeBSD 13, everything looks >good so far except LLVM JIT, which fails with various "Cannot select" >errors. Clang works fine for compiling PostgreSQL itself. Tested >with LLVM 12 (LLVM has supported RISCV since 9). Example: > >+FATAL: fatal llvm error: Cannot select: 0x4f772068: ch = brcond >0x4f770f70, 0x4f772208, BasicBlock:ch< 0x4f76d600> >+ 0x4f772208: i64 = setcc 0x4f7723a8, Constant:i64<0>, setlt:ch >+0x4f7723a8: i64,ch = load<(load 4 from `i32* inttoptr (i64 >1260491408 to i32*)`, align 16), sext from i32> 0x4fdee058, >Constant:i64<1260491408>, undef:i64 >+ 0x4f770a90: i64 = Constant<1260491408> >+ 0x4f7703a8: i64 = undef >+0x4f7701a0: i64 = Constant<0> >+In function: evalexpr_0_0 Any chance you could enable jit_dump_bitcode and manually try a failing query? That should dump. bc files in the data directory. That'd might allow debugging this outside the emulated environment. I don't see where the undef is originating from, but I think it might suggest that something lead to that code being considered unreachable. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH] Native spinlock support on RISC-V
On Wed, Sep 1, 2021 at 9:22 PM Christoph Berg wrote: > Re: Tom Lane > > I did not like confusing the RISC-V case with the ARM case: duplicating > > the code block seems better, in case there's ever reason to add > > arch-specific stuff like SPIN_DELAY. So I split it off to its own > > code block and pushed it. > > Fwiw I can report success on Debian's riscv port with that commit > cherry-picked onto 13.4: > > https://buildd.debian.org/status/fetch.php?pkg=postgresql-13=riscv64=13.4-3=1630411643=0 A couple of things I noticed on this architecture: 1. Even though we're using generic built-ins for atomics, I guess we could still use a src/include/port/atomics/arch-riscv.h file so we have a place to define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY when building for 64 bit. We'd need to find the chapter-and-verse to justify that, of course, which I can try to do; if someone more familiar with the ISA/spec/manual can point to it I'm all ears. 2. When configured with all options on FreeBSD 13, everything looks good so far except LLVM JIT, which fails with various "Cannot select" errors. Clang works fine for compiling PostgreSQL itself. Tested with LLVM 12 (LLVM has supported RISCV since 9). Example: +FATAL: fatal llvm error: Cannot select: 0x4f772068: ch = brcond 0x4f770f70, 0x4f772208, BasicBlock:ch< 0x4f76d600> + 0x4f772208: i64 = setcc 0x4f7723a8, Constant:i64<0>, setlt:ch +0x4f7723a8: i64,ch = load<(load 4 from `i32* inttoptr (i64 1260491408 to i32*)`, align 16), sext from i32> 0x4fdee058, Constant:i64<1260491408>, undef:i64 + 0x4f770a90: i64 = Constant<1260491408> + 0x4f7703a8: i64 = undef +0x4f7701a0: i64 = Constant<0> +In function: evalexpr_0_0
Re: [PATCH] Native spinlock support on RISC-V
Re: Tom Lane > I did not like confusing the RISC-V case with the ARM case: duplicating > the code block seems better, in case there's ever reason to add > arch-specific stuff like SPIN_DELAY. So I split it off to its own > code block and pushed it. Fwiw I can report success on Debian's riscv port with that commit cherry-picked onto 13.4: https://buildd.debian.org/status/fetch.php?pkg=postgresql-13=riscv64=13.4-3=1630411643=0 Christoph
Re: [PATCH] Native spinlock support on RISC-V
Daniel Gustafsson writes: > There didn’t seem to be anything left here (at least until there is hardware > in > the buildfarm) so I took the liberty to close it as committed in the CF app. Ah, sorry, I did not realize there was a CF entry. regards, tom lane
Re: [PATCH] Native spinlock support on RISC-V
> On 13 Aug 2021, at 20:09, Tom Lane wrote: > ..I split it off to its own code block and pushed it. There didn’t seem to be anything left here (at least until there is hardware in the buildfarm) so I took the liberty to close it as committed in the CF app. -- Daniel Gustafsson https://vmware.com/
Re: [PATCH] Native spinlock support on RISC-V
Andres Freund writes: > On 2021-08-13 13:37:02 -0400, Tom Lane wrote: >> so it seems like someday we might want to expend some effort on native >> atomics. I agree that that day need not be today, though. This patch >> seems sufficient until we get to the point of (at least) having some >> RISC-V in the buildfarm. > Gcc's intriniscs are pretty good these days (and if not, a *lot* of > projects just outright break). > What's more, It turns out that using intrinsics with compilers of the > last ~5 years often generates *better* code than inline assembly > (e.g. because the compiler can utilize condition codes more effectively > and has more detailed information about clobbers). So for new platforms > that'll only have support by new compilers it doesn't really make sense > to add inline assembler imo. I didn't say it had to be __asm blocks ;-). I was thinking more of the sort of stuff we have in e.g. arch-arm.h and arch-ia64.h, where we know some things about what is efficient or less efficient on a particular architecture. gcc will do its best to provide implementations of its builtins, but that doesn't mean that using a particular one of them is always the most sane thing to do. But anyway, that seems like minor optimization that maybe someday somebody will be motivated to do. We're on the same page about this being enough for today. I did not like confusing the RISC-V case with the ARM case: duplicating the code block seems better, in case there's ever reason to add arch-specific stuff like SPIN_DELAY. So I split it off to its own code block and pushed it. regards, tom lane
Re: [PATCH] Native spinlock support on RISC-V
Hi, On 2021-08-13 13:37:02 -0400, Tom Lane wrote: > "Andres Freund" writes: > > On Fri, Aug 13, 2021, at 19:25, Tom Lane wrote: > >> I now have looked at the patch, and it seems good as far as it goes, > >> but I wonder whether some effort ought to be expended in > >> src/include/port/atomics/. > > > That should automatically pick up the intrinsic. I think we should do the > > same on modern compilers for spinlocks, but that's a separate discussion I > > guess. > > I was looking at the comment in atomics.h about > > * Provide a full fallback of the pg_*_barrier(), pg_atomic**_flag and > * pg_atomic_* APIs for platforms without sufficient spinlock and/or atomics > * support. In the case of spinlock backed atomics the emulation is expected > * to be efficient, although less so than native atomics support. > > so it seems like someday we might want to expend some effort on native > atomics. I agree that that day need not be today, though. This patch > seems sufficient until we get to the point of (at least) having some > RISC-V in the buildfarm. For gcc compatible compilers the relevant comments would be * There exist generic, hardware independent, implementations for several * compilers which might be sufficient, although possibly not optimal, for a * new platform. If no such generic implementation is available spinlocks (or * even OS provided semaphores) will be used to implement the API. and /* * Compiler specific, but architecture independent implementations. * * Provide architecture independent implementations of the atomic * facilities. At the very least compiler barriers should be provided, but a * full implementation of * * pg_compiler_barrier(), pg_write_barrier(), pg_read_barrier() * * pg_atomic_compare_exchange_u32(), pg_atomic_fetch_add_u32() * using compiler intrinsics are a good idea. */ Gcc's intriniscs are pretty good these days (and if not, a *lot* of projects just outright break). What's more, It turns out that using intrinsics with compilers of the last ~5 years often generates *better* code than inline assembly (e.g. because the compiler can utilize condition codes more effectively and has more detailed information about clobbers). So for new platforms that'll only have support by new compilers it doesn't really make sense to add inline assembler imo. Greetings, Andres Freund
Re: [PATCH] Native spinlock support on RISC-V
"Andres Freund" writes: > On Fri, Aug 13, 2021, at 19:25, Tom Lane wrote: >> I now have looked at the patch, and it seems good as far as it goes, >> but I wonder whether some effort ought to be expended in >> src/include/port/atomics/. > That should automatically pick up the intrinsic. I think we should do the > same on modern compilers for spinlocks, but that's a separate discussion I > guess. I was looking at the comment in atomics.h about * Provide a full fallback of the pg_*_barrier(), pg_atomic**_flag and * pg_atomic_* APIs for platforms without sufficient spinlock and/or atomics * support. In the case of spinlock backed atomics the emulation is expected * to be efficient, although less so than native atomics support. so it seems like someday we might want to expend some effort on native atomics. I agree that that day need not be today, though. This patch seems sufficient until we get to the point of (at least) having some RISC-V in the buildfarm. regards, tom lane
Re: [PATCH] Native spinlock support on RISC-V
Hi, On Fri, Aug 13, 2021, at 19:25, Tom Lane wrote: > I wrote: > > Andres Freund writes: > >> Should we backpatch this? It's not like we're going to break existing > >> risc-v systems by enabling spinlock support... > > > Yeah, why not? If you were building with --disable-spinlocks before, > > this shouldn't change anything for you. > > (I haven't actually looked at the patch, mind you, but in principle > > it shouldn't break anything that worked before.) > > I now have looked at the patch, and it seems good as far as it goes, > but I wonder whether some effort ought to be expended in > src/include/port/atomics/. That should automatically pick up the intrinsic. I think we should do the same on modern compilers for spinlocks, but that's a separate discussion I guess. Address
Re: [PATCH] Native spinlock support on RISC-V
I wrote: > Andres Freund writes: >> Should we backpatch this? It's not like we're going to break existing >> risc-v systems by enabling spinlock support... > Yeah, why not? If you were building with --disable-spinlocks before, > this shouldn't change anything for you. > (I haven't actually looked at the patch, mind you, but in principle > it shouldn't break anything that worked before.) I now have looked at the patch, and it seems good as far as it goes, but I wonder whether some effort ought to be expended in src/include/port/atomics/. regards, tom lane
Re: [PATCH] Native spinlock support on RISC-V
Andres Freund writes: > On 2021-08-13 11:09:04 -0400, Tom Lane wrote: >> Marek Szuba writes: >>> Tested against PostgreSQL 13.3 on a physical rv64gc system (BeagleV >>> Starlight beta board) - builds and installs fine, all tests pass. > Should we backpatch this? It's not like we're going to break existing > risc-v systems by enabling spinlock support... Yeah, why not? If you were building with --disable-spinlocks before, this shouldn't change anything for you. (I haven't actually looked at the patch, mind you, but in principle it shouldn't break anything that worked before.) regards, tom lane
Re: [PATCH] Native spinlock support on RISC-V
Hi, On 2021-08-13 11:09:04 -0400, Tom Lane wrote: > Marek Szuba writes: > > Tested against PostgreSQL 13.3 on a physical rv64gc system (BeagleV > > Starlight beta board) - builds and installs fine, all tests pass. Seems like a good idea to me. > Cool ... I had hoped to acquire one of those myself, but I didn't > make the cut. Should we backpatch this? It's not like we're going to break existing risc-v systems by enabling spinlock support... Greetings, Andres Freund
Re: [PATCH] Native spinlock support on RISC-V
Marek Szuba writes: > Tested against PostgreSQL 13.3 on a physical rv64gc system (BeagleV > Starlight beta board) - builds and installs fine, all tests pass. Cool ... I had hoped to acquire one of those myself, but I didn't make the cut. regards, tom lane
[PATCH] Native spinlock support on RISC-V
Hello, The attached patch adds native spinlock support to PostgreSQL on RISC-V systems. As suspected by Richard W.M. Jones of Red Hat back in 2016, the __sync_lock_test_and_set() approach applied on arm and arm64 works here as well. Tested against PostgreSQL 13.3 on a physical rv64gc system (BeagleV Starlight beta board) - builds and installs fine, all tests pass. From what I can see in gcc documentation this should in theory work on rv32 (and possibly rv128) as well, therefore the patch as it stands covers all RISC-V systems (i.e. doesn't check the value of __risc_xlen) - but I haven't confirmed this experimentally. -- MS --- a/src/include/storage/s_lock.h +++ b/src/include/storage/s_lock.h @@ -315,12 +315,12 @@ #endif /* __ia64__ || __ia64 */ /* - * On ARM and ARM64, we use __sync_lock_test_and_set(int *, int) if available. + * On ARM, ARM64 and RISC-V, we use __sync_lock_test_and_set(int *, int) if available. * * We use the int-width variant of the builtin because it works on more chips * than other widths. */ -#if defined(__arm__) || defined(__arm) || defined(__aarch64__) || defined(__aarch64) +#if defined(__arm__) || defined(__arm) || defined(__aarch64__) || defined(__aarch64) || defined(__riscv) #ifdef HAVE_GCC__SYNC_INT32_TAS #define HAS_TEST_AND_SET @@ -337,7 +337,7 @@ #define S_UNLOCK(lock) __sync_lock_release(lock) #endif /* HAVE_GCC__SYNC_INT32_TAS */ -#endif /* __arm__ || __arm || __aarch64__ || __aarch64 */ +#endif /* __arm__ || __arm || __aarch64__ || __aarch64 || __riscv */ /* S/390 and S/390x Linux (32- and 64-bit zSeries) */ OpenPGP_signature Description: OpenPGP digital signature