This is an archive of the discontinued LLVM Phabricator instance.

Don't generate inline atomics for i386/i486
ClosedPublic

Authored by wmi on Jan 16 2018, 5:40 PM.

Details

Summary

This is to fix the bug reported in https://bugs.llvm.org/show_bug.cgi?id=34347#c6

Currently, all MaxAtomicInlineWidth of x86-32 targets are set to 64. However, i386 doesn't support any cmpxchg related instructions. i486 only supports cmpxchg. So in this patch MaxAtomicInlineWidth is reset as follows.
For i486, the MaxAtomicInlineWidth should be 32 because it supports cmpxchg.
For i386, the MaxAtomicInlineWidth should be 0.
For others x86-32 cpu, the MaxAtomicInlineWidth should be 64 because of cmpxchg8b.

Diff Detail

Repository
rC Clang

Event Timeline

wmi created this revision.Jan 16 2018, 5:40 PM

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

lib/Basic/Targets/X86.h
472

What exactly does "CK_Generic" mean here? Under what circumstances does it show up? We should have a specific default for every supported target, if user doesn't specify a CPU with -march.

craig.topper added inline comments.
lib/Basic/Targets/X86.h
472

CK_Generic is the default for 32-bit mode with no -march. CK_x86_64 is the default for 64-bit mode with no march.

I'm not sure we can assume CK_Generic is 586 or better. We don't assume that in the preprocessor defines.

if (CPU >= CK_i486) {
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1");
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2");
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4");
}
if (CPU >= CK_i586)
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8");
if (HasCX16)
  Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16");
craig.topper added inline comments.Jan 16 2018, 6:28 PM
lib/Basic/Targets/X86.h
472

Maybe we don't default to "generic" anywhere. Its very platform specific. getX86TargetCPU in lib/Driver/ToolChains/Arch/X86.cpp contains the magic I think.

wmi added a comment.Jan 16 2018, 6:33 PM

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

With the patch, for i386/i486 targets, clang will generate more atomic libcalls than before, for which llvm backend will not do anything extra, so no fix is necessary in llvm backend for the patch to work.

lib/Basic/Targets/X86.h
472

Like the command "clang -cc1 -triple=i686-linux-gnu", the cpu will be set to CK_Generic. I want to set the MaxAtomicInlineWidth of the default target to 64 (same as before) to minimize the churn. In the patch, MaxAtomicInlineWidth will only be changed if i386/i486 are explicitly specified.

wmi added inline comments.Jan 16 2018, 6:42 PM
lib/Basic/Targets/X86.h
472

In X86TargetInfo::getCPUKind (tools/clang/lib/Basic/Targets/X86.cpp), we default to "generic", for both 32 bits and 64 bits modes.

craig.topper added inline comments.Jan 16 2018, 6:54 PM
lib/Basic/Targets/X86.h
472

You're right. Without the driver we don't have a CPU string to use and end up defaulting to CK_Generic.

In D42154#977991, @wmi wrote:

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

With the patch, for i386/i486 targets, clang will generate more atomic libcalls than before, for which llvm backend will not do anything extra, so no fix is necessary in llvm backend for the patch to work.

I think Eli's point is that we do not currently support generating code for the 386 and 486 because there are other things in the x86 backend that assume that the target is at minimum a Pentium. If you're looking to support targeting those chips, you should look into that.

I don't have any objections to this patch.

wmi added a comment.Jan 22 2018, 9:32 AM
In D42154#977991, @wmi wrote:

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

With the patch, for i386/i486 targets, clang will generate more atomic libcalls than before, for which llvm backend will not do anything extra, so no fix is necessary in llvm backend for the patch to work.

I think Eli's point is that we do not currently support generating code for the 386 and 486 because there are other things in the x86 backend that assume that the target is at minimum a Pentium. If you're looking to support targeting those chips, you should look into that.

I am not looking to support those chips. Just because https://reviews.llvm.org/rL314145 changed the status of FreeBSD ports on 32 bits x86 as reported in https://bugs.llvm.org/show_bug.cgi?id=34347#c6, I want to provide a workaround and give them a relief.
This is also the intention of keeping MaxAtomicInlineWidth to be 64 for CK_Generic. In this way, it provides the minimum churn for current status -- otherwise at least a bunch of tests need to be fixed.

rjmccall accepted this revision.Jan 22 2018, 5:29 PM

Well, my point is that the example in the linked bug is asking for 486 code-generation, which is apparently unsupported by LLVM.

Anyway, it's not a good reason to hold up this patch, since the way to support things is to gradually fix the bugs with them. LGTM.

This revision is now accepted and ready to land.Jan 22 2018, 5:29 PM
This revision was automatically updated to reflect the committed changes.