Page MenuHomePhabricator

Set MaxAtomicInlineWidth properly for i386, i486, and x86-64 cpus without cmpxchg16b.
Needs ReviewPublic

Authored by jyknight on Mar 7 2016, 8:01 AM.

Details

Reviewers
t.p.northover
Summary

Also, remove all the manual definition of
GCC_HAVE_SYNC_COMPARE_AND_SWAP_* macros. Instead, define it based on
MaxAtomicInlineWidth, just like the
GCC_ATOMIC_*_LOCK_FREE macros.

Previously, the __GCC_HAVE_SYNC_COMPARE_AND_SWAP_* defines had been
computed separately, and *were* being set appropriately for older x86
targets, while MaxAtomicInlineWidth was not.

(Note the one FIXME comment: __GCC_ATOMIC_LLONG_LOCK_FREE is (still) set
improperly for i586, due to checking alignment where it ought not to.)

Test changes of note:

A few test invocations of clang -cc1 needed to have a "-target-cpu i686"
argument added -- the Driver would do so automatically based on the
"i686" in the triple, but in the tests cases cc1 invocations, it was
missing.

Some of the OpenMP tests needed a "-target-cpu core2" added: they were
assuming that cmpxchg16b was available, even though it's not in the base
x86_64 architecture.

An ARM test was asserting that V8M doesn't have
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1. This was wrong: V8M *does* support
atomic operations.

Diff Detail

Event Timeline

jyknight updated this revision to Diff 49965.Mar 7 2016, 8:01 AM
jyknight retitled this revision from to Set MaxAtomicInlineWidth properly for i386, i486, and x86-64 cpus without cmpxchg16b..
jyknight updated this object.
jyknight added a reviewer: t.p.northover.
jyknight added a subscriber: cfe-commits.
rsmith added a subscriber: rsmith.Mar 31 2016, 3:34 PM
rsmith added inline comments.
lib/Basic/Targets.cpp
4083

... where by 64, you mean 8, right? :)

test/CodeGen/atomic-ops.c
1

Why do you need a -target-cpu i686 in addition to the i686 triple?

test/Preprocessor/arm-target-features.c
87–103

Maybe drop the #define on these lines for consistency with the surrounding tests?

test/Preprocessor/init.c
3327

Can you add a MIPSN32BE-NOT: for the 16 byte form? Likewise for the below cases.

jyknight marked an inline comment as done.Apr 1 2016, 2:56 PM
jyknight added inline comments.
test/CodeGen/atomic-ops.c
1

The Driver code is what's responsible for parsing the default target cpu out of the triple. So if you invoke CC1 yourself, and don't pass -target-cpu, it treats i686-* triples as still targeting i386.

test/Preprocessor/arm-target-features.c
87–103

Did the inverse (cleaned up the surrounding lines) in r265187.

test/Preprocessor/init.c
3327

Done here and everywhere.

jyknight updated this revision to Diff 52430.Apr 1 2016, 2:57 PM

Review tweaks.

majnemer added inline comments.
lib/Basic/Targets.cpp
2565–2577

Might be easier to read if we go from increasing strength:

void setAtomic() {
  MaxAtomicInlineWidth = 0;
  if (CPU >= CK_i486)
    MaxAtomicInlineWidth = 32;
  if (CPU >= CK_i586)
    MaxAtomicInlineWidth = 64;
  if (getTriple().getArch() == llvm::Triple::x86_64) {
    MaxAtomicInlineWidth = 64;
    if (HasCX16)
      MaxAtomicInlineWidth = 128;
  }
}

Either works for me.

dsanders added inline comments.
test/Preprocessor/init.c
3327

I've just noticed that we only check the '__GCC_*' macros for the 64-bit ABI's (N32/N64). I'm not sure why the 32-bit (O32) checks are missing.

The O32 cases are the same as N32 except that __GCC_ATOMIC_LLONG_LOCK_FREE is 1 and __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 is not defined.
Could you add:

// MIPS32LE: #define __GCC_ATOMIC_BOOL_LOCK_FREE 2
// MIPS32LE: #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
// MIPS32LE: #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
// MIPS32LE: #define __GCC_ATOMIC_CHAR_LOCK_FREE 2
// MIPS32LE: #define __GCC_ATOMIC_INT_LOCK_FREE 2
// MIPS32LE: #define __GCC_ATOMIC_LLONG_LOCK_FREE 1
// MIPS32LE: #define __GCC_ATOMIC_LONG_LOCK_FREE 2
// MIPS32LE: #define __GCC_ATOMIC_POINTER_LOCK_FREE 2
// MIPS32LE: #define __GCC_ATOMIC_SHORT_LOCK_FREE 2
// MIPS32LE: #define __GCC_ATOMIC_TEST_AND_SET_TRUEVAL 1
// MIPS32LE: #define __GCC_ATOMIC_WCHAR_T_LOCK_FREE 2
// MIPS32LE: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
// MIPS32LE: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
// MIPS32LE: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
// MIPS32LE-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8
// MIPS32LE-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16

and the same for the MIPS32BE case?

jyknight updated this revision to Diff 52587.Apr 4 2016, 11:28 AM
jyknight marked 2 inline comments as done.

review fixes

Done and done.