Page MenuHomePhabricator

[Power] Use AtomicExpandPass for fence insertion, and use lwsync where appropriate
ClosedPublic

Authored by morisset on Sep 3 2014, 4:23 PM.

Details

Summary

This patch makes use of AtomicExpandPass in Power for inserting fences around
atomic as part of an effort to remove fence insertion from SelectionDAGBuilder.
As a big bonus, it lets us use sync 1 (lightweight sync, often used by the mnemonic
lwsync) instead of sync 0 (heavyweight sync) in many cases.

I also added a test, as there was no test for the barriers emitted by the Power
backend for atomic loads and stores.

Diff Detail

Repository
rL LLVM

Event Timeline

morisset updated this revision to Diff 13234.Sep 3 2014, 4:23 PM
morisset retitled this revision from to [Power] Use AtomicExpandPass for fence insertion, and use lwsync where appropriate.
morisset updated this object.
morisset edited the test plan for this revision. (Show Details)
morisset added a reviewer: jfb.
morisset added a subscriber: Unknown Object (MLST).
hfinkel added a subscriber: hfinkel.Sep 4 2014, 8:30 AM
hfinkel added inline comments.
lib/Target/PowerPC/PPCISelLowering.h
412 ↗(On Diff #13234)

The indentation here seems odd.

414 ↗(On Diff #13234)

Here too.

lib/Target/PowerPC/PPCInstrInfo.td
1710 ↗(On Diff #13234)

This is correct, as such, but should really be enhanced. The PPC A2, for example, supports SYNC (even though it is a Book E processor), such that MSYNC is really just SYNC 0. Older Book E designs, like the PPC 440, only support MSYNC. We'll want to split SYNC support into its own feature.

lib/Target/PowerPC/PPCTargetMachine.cpp
105 ↗(On Diff #13234)

Don't need a blank line here.

morisset updated this revision to Diff 13275.Sep 4 2014, 10:34 AM

Fix formatting based on hfinkel comments.

About replacing the BookE flag by a special flag for the sync instruction,
should I try doing it in this patch, or is it best as a separate thing ?

  • Original Message -----

From: "Robin Morisset" <morisset@google.com>
To: morisset@google.com, jfb@chromium.org
Cc: hfinkel@anl.gov, llvm-commits@cs.uiuc.edu
Sent: Thursday, September 4, 2014 12:34:15 PM
Subject: Re: [PATCH] [Power] Use AtomicExpandPass for fence insertion, and use lwsync where appropriate

Fix formatting based on hfinkel comments.

About replacing the BookE flag by a special flag for the sync
instruction,
should I try doing it in this patch, or is it best as a separate
thing ?

Let's do that in a follow-up commit. Please add a FIXME in this patch so we remember to do it.

Thanks again,
Hal

http://reviews.llvm.org/D5180

Files:

include/llvm/IR/IntrinsicsPowerPC.td
lib/Target/PowerPC/PPCISelLowering.cpp
lib/Target/PowerPC/PPCISelLowering.h
lib/Target/PowerPC/PPCInstrInfo.td
lib/Target/PowerPC/PPCTargetMachine.cpp
test/CodeGen/PowerPC/atomics.ll
morisset updated this revision to Diff 13276.Sep 4 2014, 10:40 AM

Add a Fixme about the use of IsBookE flag for sync support.

Minor typo on line 6558

lib/Target/PowerPC/PPCISelLowering.cpp
6558 ↗(On Diff #13276)

dependant => dependent

morisset updated this revision to Diff 13288.Sep 4 2014, 3:17 PM

Fix typo and add comment.

I've reviewed this against what gcc does, and your emitLeadingFence matches exactly. As you've noted, what you're doing for emitTrailingFence is conservative. GCC generates an isync in rs6000.c:rs6000_post_atomic_barrier() for these cases.

In short, the patch LGTM, and you can improve it later if desired.

wschmidt accepted this revision.Sep 5 2014, 8:40 AM
wschmidt added a reviewer: wschmidt.
This revision is now accepted and ready to land.Sep 5 2014, 8:40 AM
morisset closed this revision.Sep 23 2014, 1:57 PM
morisset updated this revision to Diff 14017.

Closed by commit rL218331 (authored by @morisset).