[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

rprichard added subscribers: craig.topper, rprichard. rprichard added a comment. Herald added a project: All. Maybe this change is obsolete now that D59566 is merged? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D29542/new/

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny marked 5 inline comments as done. mgorny added a comment. A gentle ping. https://reviews.llvm.org/D29542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny added inline comments. Comment at: test/CodeGen/atomic-ops.c:1 -// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map -triple=i686-apple-darwin9 | FileCheck %s +// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

hans added a reviewer: jyknight. hans added a comment. +jyknight, who had a similar patch in http://reviews.llvm.org/D17933 (see also r291477 and PR31864) Comment at: test/CodeGen/atomic-ops.c:1 -// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny added a comment. Le gentle ping. https://reviews.llvm.org/D29542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny updated this revision to Diff 87311. mgorny added a comment. Removed the i386 branch. Now the i486+ are used unconditionally. https://reviews.llvm.org/D29542 Files: lib/Basic/Targets.cpp test/CodeGen/atomic-ops.c test/CodeGen/ms-volatile.c test/CodeGenCXX/atomicinit.cpp

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

efriedma added a comment. I think you're running into https://llvm.org/bugs/show_bug.cgi?id=31620 . https://reviews.llvm.org/D29542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

dim added a comment. There's still something strange here. If I compile the following on i386-freebsd12, with `clang -march=i486 -O2 -S`: _Atomic(long long) ll; void f(void) { ++ll; } the result is: .globl f .p2align4, 0x90 .type f,@function

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

jlebar added a comment. > Well, the thing is, we don't call HostTarget->setCPU() before this function. > We just call AllocateTarget(), and it does not set the CPU. Ah, got it. LGTM for the nvptx changes. https://reviews.llvm.org/D29542 ___

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny added inline comments. Comment at: lib/Basic/Targets.cpp:4244 +} else // allow locked atomics up to 4 bytes + MaxAtomicPromoteWidth = 32; + } ahatanak wrote: > mgorny wrote: > > dim wrote: > > > Are you purposefully not setting

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

ahatanak added inline comments. Comment at: lib/Basic/Targets.cpp:4244 +} else // allow locked atomics up to 4 bytes + MaxAtomicPromoteWidth = 32; + } mgorny wrote: > dim wrote: > > Are you purposefully not setting `MaxAtomicInlineWidth` here? (It

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

joerg added a comment. Generic should imply i486+. I don't think any general purpose system supports i386 at this point, simply because it has an annoying number of bugs in critical components. The i486 (esp. the non-crippled ones) are reasonable easy to support and there are still people

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny added a comment. In https://reviews.llvm.org/D29542#667814, @joerg wrote: > At this point, I don't think there is any use on pretending that > i386-as-default makes sense. So I would request that the i386 case should be > made explicit or just dropped, with a preference for the latter.

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny added inline comments. Comment at: lib/Basic/Targets.cpp:1808 +if (HostTriple.getArch() == llvm::Triple::x86) + HostTarget->setCPU("i586"); + jlebar wrote: > mgorny wrote: > > jlebar wrote: > > > Okay, is this still needed now? > > Yes. I've

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

joerg added a comment. At this point, I don't think there is any use on pretending that i386-as-default makes sense. So I would request that the i386 case should be made explicit or just dropped, with a preference for the latter. https://reviews.llvm.org/D29542

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

jlebar added inline comments. Comment at: lib/Basic/Targets.cpp:1808 +if (HostTriple.getArch() == llvm::Triple::x86) + HostTarget->setCPU("i586"); + mgorny wrote: > jlebar wrote: > > Okay, is this still needed now? > Yes. I've specifically tested with

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny added inline comments. Comment at: lib/Basic/Targets.cpp:1808 +if (HostTriple.getArch() == llvm::Triple::x86) + HostTarget->setCPU("i586"); + jlebar wrote: > Okay, is this still needed now? Yes. I've specifically tested with it commented out, and

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

jlebar added inline comments. Comment at: lib/Basic/Targets.cpp:1808 +if (HostTriple.getArch() == llvm::Triple::x86) + HostTarget->setCPU("i586"); + Okay, is this still needed now? https://reviews.llvm.org/D29542

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

jlebar added inline comments. Comment at: lib/Basic/Targets.cpp:1784 + // atomics. This matches the defaults for the systems using CUDA. + // TODO: pass the host target CPU + if (HostTriple.getArch() == llvm::Triple::x86) Someone else is calling

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny updated this revision to Diff 87134. mgorny edited the summary of this revision. mgorny added a comment. Ok, this CUDA fix should be reasonable, i think. It simply assumes i586+ (i.e. all inline atomics enabled) for CUDA target builds. I seriously doubt it's technically possible that

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny added a comment. In https://reviews.llvm.org/D29542#666831, @jlebar wrote: > > Could someone help me figure out what is the cause and correct solution to > > that failure? @jlebar? > > You can see in NVPTXTargetInfo that we read properties from the host > targetinfo so that we export

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

dim added inline comments. Comment at: lib/Basic/Targets.cpp:4244 +} else // allow locked atomics up to 4 bytes + MaxAtomicPromoteWidth = 32; + } Are you purposefully not setting `MaxAtomicInlineWidth` here? (It seems from `TargetInfo` that the

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

jlebar added a comment. > Could someone help me figure out what is the cause and correct solution to > that failure? @jlebar? The test is checking that the macros have the same value when compiling for CUDA host and device. That is, if we're compiling for an x86 CPU and an NVPTX GPU, we

[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

mgorny created this revision. Herald added a subscriber: emaste. Set the maximum width of atomic operations on x86-32 based on the target CPU. The 64-bit inline atomics require cmpxchg8b which is an i586 instruction. Other inline atomics require cmpxchg which is an i486 instruction. This fixes