Align function starts to target-specific bundle alignment.

Align function starts to target-specific bundle alignment.

BUG=none
R=stichnot@chromium.org

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

[jvoung (off chromium), 2014-08-28 18:26:28]

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

[Jim Stichnoth, 2014-08-28 20:26:12]

https://codereview.chromium.org/515993002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/515993002/diff/20001/src/IceCfg.cpp#newcode317
src/IceCfg.cpp:317: Str << "\t.p2align " << getTarget()->getBundleP2Align() <<
"\n";
Should we consider padding with a halt sled, just for defense-in-depth?

https://codereview.chromium.org/515993002/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/515993002/diff/20001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:140: virtual SizeT getBundleP2Align() const = 0;
Rename this something like getBundleP2AlignLog2Bytes()?

[jvoung (off chromium), 2014-08-28 21:02:22]

https://codereview.chromium.org/515993002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/515993002/diff/20001/src/IceCfg.cpp#newcode317
src/IceCfg.cpp:317: Str << "\t.p2align " << getTarget()->getBundleP2Align() <<
"\n";
On 2014/08/28 20:26:12, stichnot wrote:
> Should we consider padding with a halt sled, just for defense-in-depth?

We could do that. Added a hook for now.

I'm not sure if there's a performance difference, if the processor does
speculative execution, or what not.

E.g., one study showed that different choices of non-executed nops could make a
difference. The guess was if it touched different processor units. Section 5.4
of
http://static.googleusercontent.com/media/research.google.com/en/us/pubs/arch...

https://codereview.chromium.org/515993002/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/515993002/diff/20001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:140: virtual SizeT getBundleP2Align() const = 0;
On 2014/08/28 20:26:12, stichnot wrote:
> Rename this something like getBundleP2AlignLog2Bytes()?

Renamed to getBundleAlignLog2Bytes() -- P2 was meant to be power of 2, which is
redundant with Log2?

https://codereview.chromium.org/515993002/diff/40001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/515993002/diff/40001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:141: virtual IceString getNonExecBundlePadding() const =
0;
IceString, or const char *?

[Jim Stichnoth, 2014-08-28 21:22:22]

Otherwise LGTM.

https://codereview.chromium.org/515993002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/515993002/diff/20001/src/IceCfg.cpp#newcode317
src/IceCfg.cpp:317: Str << "\t.p2align " << getTarget()->getBundleP2Align() <<
"\n";
On 2014/08/28 21:02:22, jvoung wrote:
> On 2014/08/28 20:26:12, stichnot wrote:
> > Should we consider padding with a halt sled, just for defense-in-depth?
> 
> We could do that. Added a hook for now.
> 
> I'm not sure if there's a performance difference, if the processor does
> speculative execution, or what not.
> 
> E.g., one study showed that different choices of non-executed nops could make
a
> difference. The guess was if it touched different processor units. Section 5.4
> of
>
http://static.googleusercontent.com/media/research.google.com/en/us/pubs/arch...

I'd kind of hope that if this performance difference really is real, it's due to
speculative execution of the never-actually-executed instructions that's
clogging up the functional units, and that speculatively executing a halt
instruction would hopefully not have that effect.

https://codereview.chromium.org/515993002/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/515993002/diff/20001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:140: virtual SizeT getBundleP2Align() const = 0;
On 2014/08/28 21:02:22, jvoung wrote:
> On 2014/08/28 20:26:12, stichnot wrote:
> > Rename this something like getBundleP2AlignLog2Bytes()?
> 
> Renamed to getBundleAlignLog2Bytes() -- P2 was meant to be power of 2, which
is
> redundant with Log2?

Ah, I didn't realize what the p2 stood for. :)  In that case, feel free to use
your original naming.

https://codereview.chromium.org/515993002/diff/40001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/515993002/diff/40001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:141: virtual IceString getNonExecBundlePadding() const =
0;
On 2014/08/28 21:02:22, jvoung wrote:
> IceString, or const char *?

I was actually wondering if it should be uint8_t.  That may be more painful to
render in textual assembly, but much easier for an integrated assembler. 
Regardless, the other issue is that we're constraining padding to single-byte
encoded instructions, but that's probably OK.

[jvoung (off chromium), 2014-08-28 22:38:13]

https://codereview.chromium.org/515993002/diff/20001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/515993002/diff/20001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:140: virtual SizeT getBundleP2Align() const = 0;
On 2014/08/28 21:22:21, stichnot wrote:
> On 2014/08/28 21:02:22, jvoung wrote:
> > On 2014/08/28 20:26:12, stichnot wrote:
> > > Rename this something like getBundleP2AlignLog2Bytes()?
> > 
> > Renamed to getBundleAlignLog2Bytes() -- P2 was meant to be power of 2, which
> is
> > redundant with Log2?
> 
> Ah, I didn't realize what the p2 stood for. :)  In that case, feel free to use
> your original naming.

Well, Log2 is probably more obvious =)

https://codereview.chromium.org/515993002/diff/40001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/515993002/diff/40001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:141: virtual IceString getNonExecBundlePadding() const =
0;
On 2014/08/28 21:22:22, stichnot wrote:
> On 2014/08/28 21:02:22, jvoung wrote:
> > IceString, or const char *?
> 
> I was actually wondering if it should be uint8_t.  That may be more painful to
> render in textual assembly, but much easier for an integrated assembler. 
> Regardless, the other issue is that we're constraining padding to single-byte
> encoded instructions, but that's probably OK.

True, we'll need the raw bytes.  I made this an ArrayRef<uint8_t>, and used
raw_ostream's write_hex().

[jvoung (off chromium), 2014-08-28 23:00:58]

Committed patchset #4 (id:60001) manually as b17f61d (presubmit successful).