Treat __sync_synchronize and asm("":::"memory") as stronger fences.

Treat __sync_synchronize and asm("":::"memory") as stronger fences.

This is a companion patch to:
  https://codereview.chromium.org/22240002/
  https://codereview.chromium.org/22474008/
and deals with the Clang-side of things.
 
The above patch will handle the fallouts of this Clang patch, including
some changes to un-duplicate work that RewriteAsmDirectives.cpp
does. The goal of this patch is to force some extra ordering on
non-atomics for le32 which LLVM doesn't necessarily provide.

R=eliben@chromium.org
TEST= ninja check-all
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3475
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3611

Committed: 3b1ef29

[jvoung (off chromium), 2013-08-05 23:17:07]

https://codereview.chromium.org/22294002/diff/1/test/CodeGen/NaCl/atomics.c
File test/CodeGen/NaCl/atomics.c (right):

https://codereview.chromium.org/22294002/diff/1/test/CodeGen/NaCl/atomics.c#n...
test/CodeGen/NaCl/atomics.c:11: // CHECK-NEXT: call void asm sideeffect "",
"~{memory}"()
Downstream in opt, does this turn into 3-fences in a row, or do all three parts
become one of the new intrinsics?

[JF, 2013-08-05 23:18:48]

https://codereview.chromium.org/22294002/diff/1/test/CodeGen/NaCl/atomics.c
File test/CodeGen/NaCl/atomics.c (right):

https://codereview.chromium.org/22294002/diff/1/test/CodeGen/NaCl/atomics.c#n...
test/CodeGen/NaCl/atomics.c:11: // CHECK-NEXT: call void asm sideeffect "",
"~{memory}"()
On 2013/08/05 23:17:07, jvoung (cr) wrote:
> Downstream in opt, does this turn into 3-fences in a row, or do all three
parts
> become one of the new intrinsics?

Right now, three fences. When I do the changes to LLVM it'll be a single
intrinsic.

[eliben, 2013-08-06 16:26:56]

Generally, we try hard to avoid Clang LOCALMODs. This needs to be upstreamed.
There's also a NextLLVMMerge tag in our issue tracker - please create an issue
with this tag to track this change's upstreaming.

https://codereview.chromium.org/22294002/diff/1/lib/CodeGen/CGBuiltin.cpp
File lib/CodeGen/CGBuiltin.cpp (right):

https://codereview.chromium.org/22294002/diff/1/lib/CodeGen/CGBuiltin.cpp#new...
lib/CodeGen/CGBuiltin.cpp:1047: if (getTarget().getTriple().getArch() ==
llvm::Triple::le32)
It would be cleaner to have a single LOCAMOD if-branch, leaving the current code
unmodified in the "else". That's easier to read (clearer what the LOCALMOD
applies to and what was there before), and easier to merge later.

[JF, 2013-08-07 17:58:28]

Updated, PTAL.

As asked, here's the issue (added to the CL description too):
  https://code.google.com/p/nativeclient/issues/detail?id=3611

https://codereview.chromium.org/22294002/diff/1/lib/CodeGen/CGBuiltin.cpp
File lib/CodeGen/CGBuiltin.cpp (right):

https://codereview.chromium.org/22294002/diff/1/lib/CodeGen/CGBuiltin.cpp#new...
lib/CodeGen/CGBuiltin.cpp:1047: if (getTarget().getTriple().getArch() ==
llvm::Triple::le32)
On 2013/08/06 16:26:56, eliben wrote:
> It would be cleaner to have a single LOCAMOD if-branch, leaving the current
code
> unmodified in the "else". That's easier to read (clearer what the LOCALMOD
> applies to and what was there before), and easier to merge later.

Done, and as suggested offline the code now uses TargetInfo/getTargetHooks.

[eliben, 2013-08-07 18:06:25]

https://codereview.chromium.org/22294002/diff/9001/lib/CodeGen/CGStmt.cpp
File lib/CodeGen/CGStmt.cpp (right):

https://codereview.chromium.org/22294002/diff/9001/lib/CodeGen/CGStmt.cpp#new...
lib/CodeGen/CGStmt.cpp:25: #include "llvm/IR/NaClAsm.h" // @LOCALMOD
This is a problem for upstreaming, no?

From the other CL I see all this new module contains is:

bool isAsmMemory(const Instruction *I);

What makes this NaCl specific? Can't you just put this utility function in a
more generic place? If llvm/IR/InlineAsm.h is not suitable, maybe
llvm/IR/InlineAsmUtils.h or something? This would allow upstreaming and avoiding
a localmod in both Clang and LLVM

https://codereview.chromium.org/22294002/diff/9001/lib/CodeGen/CGStmt.cpp#new...
lib/CodeGen/CGStmt.cpp:1706: if (getTargetHooks().asmMemoryIsFence() &&
llvm::NaCl::isAsmMemory(Result)) {
It's weird that you're going the generic way with target hooks and yet directly
query a NaCl:: function

[JF, 2013-08-07 19:48:12]

I made isAsmMemory a member of InlineAsm as suggested.

https://codereview.chromium.org/22294002/diff/9001/lib/CodeGen/CGStmt.cpp
File lib/CodeGen/CGStmt.cpp (right):

https://codereview.chromium.org/22294002/diff/9001/lib/CodeGen/CGStmt.cpp#new...
lib/CodeGen/CGStmt.cpp:25: #include "llvm/IR/NaClAsm.h" // @LOCALMOD
On 2013/08/07 18:06:25, eliben wrote:
> This is a problem for upstreaming, no?
> 
> From the other CL I see all this new module contains is:
> 
> bool isAsmMemory(const Instruction *I);
> 
> What makes this NaCl specific? Can't you just put this utility function in a
> more generic place? If llvm/IR/InlineAsm.h is not suitable, maybe
> llvm/IR/InlineAsmUtils.h or something? This would allow upstreaming and
avoiding
> a localmod in both Clang and LLVM

Done.

https://codereview.chromium.org/22294002/diff/9001/lib/CodeGen/CGStmt.cpp#new...
lib/CodeGen/CGStmt.cpp:1706: if (getTargetHooks().asmMemoryIsFence() &&
llvm::NaCl::isAsmMemory(Result)) {
On 2013/08/07 18:06:25, eliben wrote:
> It's weird that you're going the generic way with target hooks and yet
directly
> query a NaCl:: function

Done.

[eliben, 2013-08-07 20:36:34]

lgtm

[JF, 2013-08-07 22:50:48]

Committed patchset #3 manually as r3b1ef29 (presubmit successful).