First pass at emitIAS for branches and binding labels

First pass at emitIAS for branches and binding labels

Currently not testing fixups of forward branches and
instead streaming a ".byte (foo - (. + 1))" or
".long (foo - (. + 4))". It should be supported once
emitIAS() delays writing things out until after the
function is fully emitted (and therefore forward labels
have all been bound).

BUG=none
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=7e1e485744b7205c2bf62356cf0903371fc82b10

[jvoung (off chromium), 2014-10-22 16:09:37]

jvoung@chromium.org changed reviewers:
+ stichnot@chromium.org

[jvoung (off chromium), 2014-10-22 16:15:00]

Patchset #3 (id:40001) has been deleted

[jvoung (off chromium), 2014-10-22 16:15:05]

Patchset #3 (id:60001) has been deleted

[Jim Stichnoth, 2014-10-22 18:01:14]

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:390: const intptr_t FwdBranchSize = Near ? 1 : 4;
Should this be size_t or ptrdiff_t or something similar?  (not sure, just
suspicious and asking)

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:461: // In all these cases, local Labels should only be
used for Near.
Is there any validation of this assumption?  Strictly speaking, it's possible
(though unlikely) for some hideously complex lowering to result in branches to
far-away local labels.  (maybe in combination with bundling, random nop
insertion, etc.)

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:277: // InstX8632Label represents an intra-block label that
is the
Document the assumption about short branches to labels.

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:333: mutable x86::Label AsmLabel; // modified by emitIAS.
Is the mutable aspect "permanent", or just a stop-gap for textual IAS emission?

https://codereview.chromium.org/673543002/diff/80001/tests_lit/assembler/x86/...
File tests_lit/assembler/x86/jump_encodings.ll (right):

https://codereview.chromium.org/673543002/diff/80001/tests_lit/assembler/x86/...
tests_lit/assembler/x86/jump_encodings.ll:165: ; Unlike forward branches to cfg
nodes, "local" forward branches
Can you add a test for a "local" backward branch?  I think this is currently
only done in expandAtomicRMWAsCmpxchg().

[jvoung (off chromium), 2014-10-22 20:43:58]

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:390: const intptr_t FwdBranchSize = Near ? 1 : 4;
On 2014/10/22 18:01:14, stichnot wrote:
> Should this be size_t or ptrdiff_t or something similar?  (not sure, just
> suspicious and asking)

Hmm, it could be a ptrdiff_t too, but I had it as intptr_t just because the
return type of Asm->GetPosition() is intptr_t, so EndPosition/StartPosition are
intptr_t. GetPosition is the diff of two uintptr_ts.

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:461: // In all these cases, local Labels should only be
used for Near.
On 2014/10/22 18:01:14, stichnot wrote:
> Is there any validation of this assumption?  Strictly speaking, it's possible
> (though unlikely) for some hideously complex lowering to result in branches to
> far-away local labels.  (maybe in combination with bundling, random nop
> insertion, etc.)

Yes, it's a bit roundabout, but when the forward-referenced label is  Bind()'ed,
there is a while loop that goes through an array of near branches. The while
loop has an assert(Utils::IsInt(8, offset)) for each of the branches.

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:277: // InstX8632Label represents an intra-block label that
is the
On 2014/10/22 18:01:14, stichnot wrote:
> Document the assumption about short branches to labels.

Done.

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:333: mutable x86::Label AsmLabel; // modified by emitIAS.
On 2014/10/22 18:01:14, stichnot wrote:
> Is the mutable aspect "permanent", or just a stop-gap for textual IAS
emission?

Unfortunately, it is permanent. I wasn't sure of a good way around this. The
issue is that emitIAS() is "const", but it is at that point when it is
convenient to store the buffer position that the AsmLabel represents, and go
through the queue of forward branch fixups needed and flush that queue.

I could store this Label w/ position and queue externally, like the case for
CfgNode labels, then it would still be const.

https://codereview.chromium.org/673543002/diff/80001/tests_lit/assembler/x86/...
File tests_lit/assembler/x86/jump_encodings.ll (right):

https://codereview.chromium.org/673543002/diff/80001/tests_lit/assembler/x86/...
tests_lit/assembler/x86/jump_encodings.ll:165: ; Unlike forward branches to cfg
nodes, "local" forward branches
On 2014/10/22 18:01:14, stichnot wrote:
> Can you add a test for a "local" backward branch?  I think this is currently
> only done in expandAtomicRMWAsCmpxchg().

Done.

[Jim Stichnoth, 2014-10-22 21:50:09]

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:333: mutable x86::Label AsmLabel; // modified by emitIAS.
On 2014/10/22 20:43:58, jvoung wrote:
> On 2014/10/22 18:01:14, stichnot wrote:
> > Is the mutable aspect "permanent", or just a stop-gap for textual IAS
> emission?
> 
> Unfortunately, it is permanent. I wasn't sure of a good way around this. The
> issue is that emitIAS() is "const", but it is at that point when it is
> convenient to store the buffer position that the AsmLabel represents, and go
> through the queue of forward branch fixups needed and flush that queue.
> 
> I could store this Label w/ position and queue externally, like the case for
> CfgNode labels, then it would still be const.

I think that would be a cleaner approach, maybe a vector parallel to
CfgNodeLabels, or make CfgNodeLabels a vector of tuples.

I just think there shouldn't be mutable or const_cast this early in the
project...

[jvoung (off chromium), 2014-10-23 18:05:27]

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:333: mutable x86::Label AsmLabel; // modified by emitIAS.
On 2014/10/22 21:50:08, stichnot wrote:
> On 2014/10/22 20:43:58, jvoung wrote:
> > On 2014/10/22 18:01:14, stichnot wrote:
> > > Is the mutable aspect "permanent", or just a stop-gap for textual IAS
> > emission?
> > 
> > Unfortunately, it is permanent. I wasn't sure of a good way around this. The
> > issue is that emitIAS() is "const", but it is at that point when it is
> > convenient to store the buffer position that the AsmLabel represents, and go
> > through the queue of forward branch fixups needed and flush that queue.
> > 
> > I could store this Label w/ position and queue externally, like the case for
> > CfgNode labels, then it would still be const.
> 
> I think that would be a cleaner approach, maybe a vector parallel to
> CfgNodeLabels, or make CfgNodeLabels a vector of tuples.
> 
> I just think there shouldn't be mutable or const_cast this early in the
> project...

Okay I agree it's cleaner. It adds a bit of indirection, but at least on my
machine the performance difference is negligible.

Done.

[jvoung (off chromium), 2014-10-23 18:19:51]

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/673543002/diff/80001/src/IceInstX8632.cpp#new...
src/IceInstX8632.cpp:461: // In all these cases, local Labels should only be
used for Near.
On 2014/10/22 20:43:58, jvoung wrote:
> On 2014/10/22 18:01:14, stichnot wrote:
> > Is there any validation of this assumption?  Strictly speaking, it's
possible
> > (though unlikely) for some hideously complex lowering to result in branches
to
> > far-away local labels.  (maybe in combination with bundling, random nop
> > insertion, etc.)
> 
> Yes, it's a bit roundabout, but when the forward-referenced label is 
Bind()'ed,
> there is a while loop that goes through an array of near branches. The while
> loop has an assert(Utils::IsInt(8, offset)) for each of the branches.

As far as stats, so far on GCC, eon, perlbmk, and the Ogre engine pexe, the
longest local label offset is 24 bytes (without bundle padding, etc. of course).

[Jim Stichnoth, 2014-10-23 19:57:31]

lgtm

[jvoung (off chromium), 2014-10-24 17:29:35]

Committed patchset #7 (id:160001) manually as
7e1e485744b7205c2bf62356cf0903371fc82b10 (presubmit successful).