This is an archive of the discontinued LLVM Phabricator instance.

[Atomic][X8664] set max atomic inline/promote width according to the target
ClosedPublic

Authored by wmi on Sep 19 2017, 11:23 AM.

Details

Summary

This is to fix PR31620. MaxAtomicPromoteWidth and MaxAtomicInlineWidth are set to 128 for x86_64. However, for target without cx16 support, 128 atomic operation will generate __sync_* libcalls. The patch set MaxAtomicPromoteWidth and MaxAtomicInlineWidth to 64 if the target doesn't support cx16.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Sep 19 2017, 11:23 AM
davide added a subscriber: davide.
efriedma edited edge metadata.Sep 19 2017, 11:35 AM

I would rather just fix clang (in lib/Basic/Targets/X86.cpp) to fix the bug, then handle the backend changes from D18201 together.

wmi retitled this revision from [AtomicExpandPass][X86] set MaxAtomicSizeInBitsSupported according to the target to [Atomic][X8664] set max atomic inline/promote width according to the target.Sep 19 2017, 6:07 PM
wmi edited the summary of this revision. (Show Details)
wmi edited subscribers, added: cfe-commits; removed: llvm-commits.
wmi updated this revision to Diff 115945.Sep 19 2017, 6:09 PM

Address Eli's comment.

Needs testcase.

lib/Basic/Targets/X86.h
898 ↗(On Diff #115945)

I don't think we need to mess with MaxAtomicPromoteWidth?

Probably more intuitive to check "if (hasFeature" rather than "if (!hasFeature".

Adding a dedicated hook for this seems a bit overkill, but I don't have a better suggestion.

wmi added inline comments.Sep 20 2017, 2:43 PM
lib/Basic/Targets/X86.h
898 ↗(On Diff #115945)

If 128 bits inline atomic is not supported, what is the point to promote atomic type to 128 bits?

efriedma added inline comments.Sep 20 2017, 2:56 PM
lib/Basic/Targets/X86.h
898 ↗(On Diff #115945)

MaxAtomicPromoteWidth affects the ABI, so it can't vary based on the target CPU.

wmi added inline comments.Sep 20 2017, 4:08 PM
lib/Basic/Targets/X86.h
898 ↗(On Diff #115945)

That make senses. Thanks for the explanation.

wmi updated this revision to Diff 116107.Sep 20 2017, 4:09 PM

Address Eli's comments.

This revision is now accepted and ready to land.Sep 21 2017, 12:07 PM
This revision was automatically updated to reflect the committed changes.