Add the new @llvm.nacl.atomic.fence.all intrinsic

Add the new @llvm.nacl.atomic.fence.all intrinsic

This is a follow-up to:
  https://codereview.chromium.org/22240002/
And requires the Clang changes from:
  https://codereview.chromium.org/22294002/

This new intrinsic represents ``asm("":::"~{memory}")`` as well as ``__sync_synchronize()``, and in IR it corresponds to a sequentially-consistent fence surrounded by ``call void asm sideeffect "", "~{memory}"()``.

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

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=10c5d2c

[jvoung (off chromium), 2013-08-07 19:38:40]

https://codereview.chromium.org/22474008/diff/1/lib/IR/NaClAtomicIntrinsics.cpp
File lib/IR/NaClAtomicIntrinsics.cpp (right):

https://codereview.chromium.org/22474008/diff/1/lib/IR/NaClAtomicIntrinsics.c...
lib/IR/NaClAtomicIntrinsics.cpp:57: INIT(Mem, NoP, NoP, NoP, NoP, fence);
I think I brought this up before -- it seems a bit wasteful to create versions
of fence and fence_all for each overload type, even though there aren't
overloads.

Doesn't it mean that the transform pass will loop through each call in a
function 3x more than needed for each of the fence and fence_all?


The loop:

NaCl::AtomicIntrinsics::View V = AI.allIntrinsicsAndOverloads();
for (NaCl::AtomicIntrinsics::View::iterator I = V.begin(), E = V.end(); I != E;
++I) {
    AtomicCallResolver AtomicResolver(F, I);
    Changed |= visitCalls(AtomicResolver);
}

https://codereview.chromium.org/22474008/diff/1/lib/Transforms/NaCl/RewriteAt...
File lib/Transforms/NaCl/RewriteAtomics.cpp (right):

https://codereview.chromium.org/22474008/diff/1/lib/Transforms/NaCl/RewriteAt...
lib/Transforms/NaCl/RewriteAtomics.cpp:332: ///   call void
@llvm.nacl.atomic.fence.all()
Is it possible to just have this pass remove the asm ~{memory}, and instead we
having an extra pass?

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

I made isAsmMemory a member of InlineAsm as suggested in the companion CL:
  https://codereview.chromium.org/22294002/

[JF, 2013-08-07 19:55:21]

https://codereview.chromium.org/22474008/diff/1/lib/IR/NaClAtomicIntrinsics.cpp
File lib/IR/NaClAtomicIntrinsics.cpp (right):

https://codereview.chromium.org/22474008/diff/1/lib/IR/NaClAtomicIntrinsics.c...
lib/IR/NaClAtomicIntrinsics.cpp:57: INIT(Mem, NoP, NoP, NoP, NoP, fence);
On 2013/08/07 19:38:40, jvoung (cr) wrote:
> I think I brought this up before -- it seems a bit wasteful to create versions
> of fence and fence_all for each overload type, even though there aren't
> overloads.
> 
> Doesn't it mean that the transform pass will loop through each call in a
> function 3x more than needed for each of the fence and fence_all?
> 
> 
> The loop:
> 
> NaCl::AtomicIntrinsics::View V = AI.allIntrinsicsAndOverloads();
> for (NaCl::AtomicIntrinsics::View::iterator I = V.begin(), E = V.end(); I !=
E;
> ++I) {
>     AtomicCallResolver AtomicResolver(F, I);
>     Changed |= visitCalls(AtomicResolver);
> }

I can make a "default overload type" for the functions with no overload, and
then remove the duplicates from this table.

I'd rather do this in another CL, though: I'd like to get this and others in
before the branch. If it's OK with you I'll file an issue and punt to M31.

https://codereview.chromium.org/22474008/diff/1/lib/Transforms/NaCl/RewriteAt...
File lib/Transforms/NaCl/RewriteAtomics.cpp (right):

https://codereview.chromium.org/22474008/diff/1/lib/Transforms/NaCl/RewriteAt...
lib/Transforms/NaCl/RewriteAtomics.cpp:332: ///   call void
@llvm.nacl.atomic.fence.all()
On 2013/08/07 19:38:40, jvoung (cr) wrote:
> Is it possible to just have this pass remove the asm ~{memory}, and instead we
> having an extra pass?

Could be, but I'd rather be able to keep both independent: I think it makes it
easier to test both independently.

[jvoung (off chromium), 2013-08-07 22:32:11]

lgtm

https://codereview.chromium.org/22474008/diff/1/lib/IR/NaClAtomicIntrinsics.cpp
File lib/IR/NaClAtomicIntrinsics.cpp (right):

https://codereview.chromium.org/22474008/diff/1/lib/IR/NaClAtomicIntrinsics.c...
lib/IR/NaClAtomicIntrinsics.cpp:57: INIT(Mem, NoP, NoP, NoP, NoP, fence);
On 2013/08/07 19:55:21, JF wrote:
> On 2013/08/07 19:38:40, jvoung (cr) wrote:
> > I think I brought this up before -- it seems a bit wasteful to create
versions
> > of fence and fence_all for each overload type, even though there aren't
> > overloads.
> > 
> > Doesn't it mean that the transform pass will loop through each call in a
> > function 3x more than needed for each of the fence and fence_all?
> > 
> > 
> > The loop:
> > 
> > NaCl::AtomicIntrinsics::View V = AI.allIntrinsicsAndOverloads();
> > for (NaCl::AtomicIntrinsics::View::iterator I = V.begin(), E = V.end(); I !=
> E;
> > ++I) {
> >     AtomicCallResolver AtomicResolver(F, I);
> >     Changed |= visitCalls(AtomicResolver);
> > }
> 
> I can make a "default overload type" for the functions with no overload, and
> then remove the duplicates from this table.
> 
> I'd rather do this in another CL, though: I'd like to get this and others in
> before the branch. If it's OK with you I'll file an issue and punt to M31.

ok that's fine

https://codereview.chromium.org/22474008/diff/1/lib/Transforms/NaCl/RewriteAt...
File lib/Transforms/NaCl/RewriteAtomics.cpp (right):

https://codereview.chromium.org/22474008/diff/1/lib/Transforms/NaCl/RewriteAt...
lib/Transforms/NaCl/RewriteAtomics.cpp:332: ///   call void
@llvm.nacl.atomic.fence.all()
On 2013/08/07 19:55:21, JF wrote:
> On 2013/08/07 19:38:40, jvoung (cr) wrote:
> > Is it possible to just have this pass remove the asm ~{memory}, and instead
we
> > having an extra pass?
> 
> Could be, but I'd rather be able to keep both independent: I think it makes it
> easier to test both independently.

I guess we could also make clang generate the fence_all directly, but that would
be a clang localmod.

Anyway, probably fine this way -- but you could explain here that the asm then
get erased by another pass.

https://codereview.chromium.org/22474008/diff/7001/lib/IR/InlineAsm.cpp
File lib/IR/InlineAsm.cpp (right):

https://codereview.chromium.org/22474008/diff/7001/lib/IR/InlineAsm.cpp#newco...
lib/IR/InlineAsm.cpp:304: //  - x86 "~{memory},~{dirflag},~{fpsr},~{flags}".
Does that mean that in the backend, we need to convert the fence_all back into
the target-specific "touch everything" asm ?

[JF, 2013-08-07 22:47:41]

https://codereview.chromium.org/22474008/diff/1/lib/IR/NaClAtomicIntrinsics.cpp
File lib/IR/NaClAtomicIntrinsics.cpp (right):

https://codereview.chromium.org/22474008/diff/1/lib/IR/NaClAtomicIntrinsics.c...
lib/IR/NaClAtomicIntrinsics.cpp:57: INIT(Mem, NoP, NoP, NoP, NoP, fence);
On 2013/08/07 22:32:11, jvoung (cr) wrote:
> On 2013/08/07 19:55:21, JF wrote:
> > On 2013/08/07 19:38:40, jvoung (cr) wrote:
> > > I think I brought this up before -- it seems a bit wasteful to create
> versions
> > > of fence and fence_all for each overload type, even though there aren't
> > > overloads.
> > > 
> > > Doesn't it mean that the transform pass will loop through each call in a
> > > function 3x more than needed for each of the fence and fence_all?
> > > 
> > > 
> > > The loop:
> > > 
> > > NaCl::AtomicIntrinsics::View V = AI.allIntrinsicsAndOverloads();
> > > for (NaCl::AtomicIntrinsics::View::iterator I = V.begin(), E = V.end(); I
!=
> > E;
> > > ++I) {
> > >     AtomicCallResolver AtomicResolver(F, I);
> > >     Changed |= visitCalls(AtomicResolver);
> > > }
> > 
> > I can make a "default overload type" for the functions with no overload, and
> > then remove the duplicates from this table.
> > 
> > I'd rather do this in another CL, though: I'd like to get this and others in
> > before the branch. If it's OK with you I'll file an issue and punt to M31.
> 
> ok that's fine

https://code.google.com/p/nativeclient/issues/detail?id=3612

https://codereview.chromium.org/22474008/diff/1/lib/Transforms/NaCl/RewriteAt...
File lib/Transforms/NaCl/RewriteAtomics.cpp (right):

https://codereview.chromium.org/22474008/diff/1/lib/Transforms/NaCl/RewriteAt...
lib/Transforms/NaCl/RewriteAtomics.cpp:332: ///   call void
@llvm.nacl.atomic.fence.all()
On 2013/08/07 22:32:11, jvoung (cr) wrote:
> On 2013/08/07 19:55:21, JF wrote:
> > On 2013/08/07 19:38:40, jvoung (cr) wrote:
> > > Is it possible to just have this pass remove the asm ~{memory}, and
instead
> we
> > > having an extra pass?
> > 
> > Could be, but I'd rather be able to keep both independent: I think it makes
it
> > easier to test both independently.
> 
> I guess we could also make clang generate the fence_all directly, but that
would
> be a clang localmod.
> 
> Anyway, probably fine this way -- but you could explain here that the asm then
> get erased by another pass.

Done.

https://codereview.chromium.org/22474008/diff/7001/lib/IR/InlineAsm.cpp
File lib/IR/InlineAsm.cpp (right):

https://codereview.chromium.org/22474008/diff/7001/lib/IR/InlineAsm.cpp#newco...
lib/IR/InlineAsm.cpp:304: //  - x86 "~{memory},~{dirflag},~{fpsr},~{flags}".
On 2013/08/07 22:32:11, jvoung (cr) wrote:
> Does that mean that in the backend, we need to convert the fence_all back into
> the target-specific "touch everything" asm ?

Kind of. It comes from Clang's TargetInfo getClobbers, which each target
specifies in Targets.cpp. AFAIU LLVM can't use that code, and
implementation-wise only the x86 target sets any clobbers, the others all have
FIXMEs which sound utterly confused.

My conclusion when I saw this was that what we really care about anyways is the
memory side-effects, and things like flags can be assumed to stay live.

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

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