Page MenuHomePhabricator

[Mips] Set setMaxAtomicSizeInBitsSupported
Needs ReviewPublic

Authored by brad on Jan 7 2023, 12:56 AM.

Details

Summary

Set setMaxAtomicSizeInBitsSupported for Mips. Set the value as appropriate for 64-bit MIPS vs 32-bit and take MIPS-I into consideration.

I am not quite sure if I used the appropriate Subtarget checks.

Diff Detail

Unit TestsFailed

TimeTest
60,250 msx64 debian > Clang.CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/non-overloaded::vloxseg.c
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/16/include -nostdsysteminc -triple riscv64 -target-feature +v -target-feature +zfh -target-feature +experimental-zvfh -disable-O0-optnone -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/non-overloaded/vloxseg.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -S -passes=mem2reg | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK-RV64 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/non-overloaded/vloxseg.c
60,320 msx64 debian > Clang.CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/non-overloaded::vluxseg.c
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/16/include -nostdsysteminc -triple riscv64 -target-feature +v -target-feature +zfh -target-feature +experimental-zvfh -disable-O0-optnone -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/non-overloaded/vluxseg.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -S -passes=mem2reg | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK-RV64 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/non-overloaded/vluxseg.c
60,300 msx64 debian > Clang.CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded::vloxseg.c
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/16/include -nostdsysteminc -triple riscv64 -target-feature +v -target-feature +zfh -target-feature +experimental-zvfh -disable-O0-optnone -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vloxseg.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -S -passes=mem2reg | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK-RV64 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vloxseg.c
60,250 msx64 debian > Clang.CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded::vluxseg.c
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/16/include -nostdsysteminc -triple riscv64 -target-feature +v -target-feature +zfh -target-feature +experimental-zvfh -disable-O0-optnone -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vluxseg.c -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -S -passes=mem2reg | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK-RV64 /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vluxseg.c

Event Timeline

brad created this revision.Jan 7 2023, 12:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2023, 12:56 AM
brad requested review of this revision.Jan 7 2023, 12:56 AM
brad updated this revision to Diff 489012.Jan 13 2023, 7:52 AM

Use update_llc_test_checks.py to update test.

arsenm added a subscriber: arsenm.Jan 17 2023, 12:48 PM
arsenm added inline comments.
llvm/lib/Target/Mips/MipsISelLowering.cpp
500–501

Else case doesn't make sense to me but I know nothing about mips

brad added inline comments.Jan 17 2023, 1:02 PM
llvm/lib/Target/Mips/MipsISelLowering.cpp
500–501

Else case doesn't make sense to me but I know nothing about mips

To deal with MIPS-I not having support for atomics.

wzssyqa added inline comments.Feb 3 2023, 2:06 AM
llvm/lib/Target/Mips/MipsISelLowering.cpp
500–501

Sorry for me. I am no idea about what's the mean of the value of setMaxAtomicSizeInBitsSupported.

Maybe:

  1. load/store instructions no quirky behavior: an instruction can be split to in result. If so, MIPS has never this problem since MIPS I.
  2. normal load/store instructions can make sure that the data sync in a multicore system. If so, MIPS never archives this.
  3. architecture has some instructions to help atomic ops. If so, MIPS has LL/SC since MIPS II.
brad added inline comments.Feb 3 2023, 12:15 PM
llvm/lib/Target/Mips/MipsISelLowering.cpp
500–501

setMaxAtomicSizeInBitsSupported() indicates the maximum size of supported atomic operations that are *all* lock-free. 32-bit MIPS does not have hardware support for lock-free 64-bit atomics.

wzssyqa added inline comments.Feb 6 2023, 12:34 AM
llvm/lib/Target/Mips/MipsISelLowering.cpp
500–501

Ohh, Yes, ldc1 is available since MIPS II.

and

ldc1 $f0, 0($4)
mfc1 $2, $f0
mfc1 $3, $f1

is wrong for MIPS32 FP64 ABI.
For MIPS32 FP64 ABI(support MIPS32R2+ only), it should be like:

ldc1 $f0, 0($4)
mfc1 $2, $f0
mfhc1 $3, $f0

Makes sense to me.

For MIPS 1, we have essentially two choices: either we use setMaxAtomicSizeInBitsSupported(0), and generate __atomic_* calls, or we set setMaxAtomicSizeInBitsSupported(32), and generate native load/store with __sync_* calls for cmpxchg/atomicrmw. Either way, we can't actually provide implementations; the user has to make it work somehow. See D137980 for a similar discussion... but generating __sync_* calls seems to cause less trouble. (Not sure if anyone cares about MIPS 1 these days, though.)

Hi! Sorry to jump in randomly, but it just so happens that I was working on a very similar patch right now. Funnily enough I need to make everything work on MIPS I, because there are still people caring for that target, mainly in the homebrew scene as the CPU for the Playstation 1 and coprocessor of the Playstation 2 both are MIPS I targets.

Wouldn't it be more correct to set atomic widths to 0 and generate __atomic_* calls? As just emitting sync fences on CAS/RMW operations break with multithreading/preemption? I'm not so sure about the Playstation 1 but I'm pretty sure the Playstation 2 has multithreading support on the coprocessor. Also, as it has already been pointed out, MIPS I doesn't even support sync operations, so relying on some implementation of them, to me, seems more complicated to achieve.

Wouldn't it be more correct to set atomic widths to 0 and generate __atomic_* calls?

You need some sort of operating-system level magic to make atomicrmw/cmpxchg work without ll/sc. Disabling interrupts, or a restartable sequence, or something along those lines. Most of the ways you'd implement that end up being compatible with lowering atomic load/store to plain load/store operations. Granted, other implementations are possible.

Yes exactly, what I'm saying is that if we just emit sync fences (in the form of libcalls) for CAS/RMW operations it might become impossible to target systems that have some sort of preemption. So I personally would prefer to set atomic width to 0 and generate libcalls for all atomic operations, so implementors for specific environments can do the right thing for the platform.

Baremetal Playstation 1 is likely the only MIPS1 target anyone will ever use with LLVM, so we should do whatever makes sense there. I don't think it makes sense to theorycraft configurations that don't exist. (If MIPS1 Linux were a thing, it would probably do something like https://www.kernel.org/doc/Documentation/arm/kernel_user_helpers.txt .)

Tazdevil971 added a comment.EditedFeb 8 2023, 11:45 AM

Fair enough. Looking more in detail at Playstation 1 and 2, they both use cooperative multithreading so preemption is not an issue. So for these targets it doesn't really make a difference.

brad added a comment.Mar 7 2023, 11:32 PM

Just wanted to point out this recent github bug report..

https://github.com/llvm/llvm-project/issues/61166