Suzero. X8664. NaCl Sandboxing.

Suzero. X8664. NaCl Sandboxing.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
R=sehr@chromium.org, stichnot@chromium.org

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

[John, 2016-01-12 15:32:22]

jpp@chromium.org changed reviewers:
+ kschimpf@google.com, seh@chromium.org, stichnot@chromium.org

[John, 2016-01-12 15:32:23]

And here it is. It all of is g(l)ory, x86-64 sandboxing for subzero.

[Jim Stichnoth, 2016-01-12 17:11:48]

Description was changed from

==========
Suzero. X8664. NaCl Sandboxing.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

to

==========
Suzero. X8664. NaCl Sandboxing.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

[Jim Stichnoth, 2016-01-12 17:11:48]

stichnot@chromium.org changed reviewers:
+ sehr@chromium.org
- seh@chromium.org

[Jim Stichnoth, 2016-01-12 17:12:18]

[-seh, +sehr]

[Jim Stichnoth, 2016-01-13 18:28:27]

Some initial comments.

https://codereview.chromium.org/1559243002/diff/520001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1559243002/diff/520001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:24: return flags[target]
Seems like we could just always append '-nacl' for sandboxing, and simplify this
mess.

https://codereview.chromium.org/1559243002/diff/520001/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceELFSection.h#ne...
src/IceELFSection.h:359: IceString Name = Fixup.symbol(&Ctx, Asm);
Can you move this into the "if (!Symbol) { ... }" context where it is used?

https://codereview.chromium.org/1559243002/diff/520001/src/IceELFSection.h#ne...
src/IceELFSection.h:371: // In ELF64, PC-relative relocations' addend need to
account for
reflow comment to 80-col

https://codereview.chromium.org/1559243002/diff/520001/src/IceELFSection.h#ne...
src/IceELFSection.h:371: // In ELF64, PC-relative relocations' addend need to
account for
"addends" (for agreement)

https://codereview.chromium.org/1559243002/diff/520001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceInst.cpp#newcod...
src/IceInst.cpp:863: "align_to_end /* pad_to end */";
Is the comment supposed to be "pad_to_end"? (two underscores)

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.cpp
File src/IceInstX8664.cpp (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:239: const Ice::TargetLowering *TargetLowering, bool
LeaAddr) const {
Suggestion: rename the parameter to IsLeaAddr to make the intent slightly
clearer.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:243: const bool NeedSandboxing = Target->needSandboxing();
Move this closer to the assert?

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def
File src/IceInstX8664.def (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:33: //   zeroptr: This register is used as %rzp for NaCl
sandboxing.
Sorry to do this, but could you also add the same new column to the x8632 file? 
I'm trying to keep them consistent (even if a column is useless to some target
and therefore all zeroes).

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:61: X(Reg_bpl,   5, "bpl",  Reg_rbp, 0,0,0,1,0, 1,0,0,0,0,
0, 0,0,0,1,0,         \
The "preserved" bit changed to 0, I don't think that's right.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:71: X(Reg_r10l, 10, "r10b", Reg_r10, 1,0,0,0,0, 1,0,0,0,1,
0, 0,0,0,1,0,         \
Yikes, nice catch here...

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:112: X(Reg_rbp, 5,  "rbp", Reg_rbp, 0,0,0,1,0, 1,0,0,0,0,
0, 1,0,0,0,0,           \
rbp is not a preserved register?  And it's not available for register
allocation?

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:145: X(Reg_ebp,   5, "ebp",  Reg_rbp, 0,0,0,1,0, 1,0,0,0,0,
0, 0,1,0,0,0,         \
Same questions as rbp above.  (I'm starting to see a pattern here...)

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:178: X(Reg_bp,    5, "bp",   Reg_rbp, 0,0,0,1,0, 1,0,0,0,0,
0, 0,0,1,0,0,         \
ditto

https://codereview.chromium.org/1559243002/diff/520001/src/IceLiveness.cpp
File src/IceLiveness.cpp (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceLiveness.cpp#ne...
src/IceLiveness.cpp:61: if (TmpNumGlobals != 0) {
Did you mean to leave this here?  If so, it needs to be guarded by
BuildDefs::dump() and some isVerbose() check.

https://codereview.chromium.org/1559243002/diff/520001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceTypes.def#newco...
src/IceTypes.def:25: // TODO: Whoever adds AArch64 will need to set ABI e_flags.
Since you touched this last :)
Can you add a name to the TODO?

https://codereview.chromium.org/1559243002/diff/520001/src/IceTypes.def#newco...
src/IceTypes.def:29: X(Target_X8664, "x86-64", true, EM_X86_64,  0)             
                 \
fix beautiful column alignment

https://codereview.chromium.org/1559243002/diff/520001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/callindirect.pnacl.ll (right):

https://codereview.chromium.org/1559243002/diff/520001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/callindirect.pnacl.ll:70: ; X8664-LABEL: CallIndirect
Why bother with these two RUN configs if all you're doing is checking whether
the first function translates at all?

[John, 2016-01-13 20:48:03]

https://codereview.chromium.org/1559243002/diff/520001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1559243002/diff/520001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:24: return flags[target]
On 2016/01/13 18:28:26, stichnot wrote:
> Seems like we could just always append '-nacl' for sandboxing, and simplify
this
> mess.

I thought about that, but having the complete string makes grep'ing easier.
Plus, it is just an if/else, which isn't that bad.

https://codereview.chromium.org/1559243002/diff/520001/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceELFSection.h#ne...
src/IceELFSection.h:359: IceString Name = Fixup.symbol(&Ctx, Asm);
On 2016/01/13 18:28:26, stichnot wrote:
> Can you move this into the "if (!Symbol) { ... }" context where it is used?

Having this here allows for a better error-message. And this condition (!Symbol)
should only happen here -- I honestly believe it's better to have this error
checking here:

1 - If a symbol is not found, then the error message will say which symbol that
was.

2 - If Symbol is null, uses of Symbol will segfault, and we will know something
is wrong elsewhere.

https://codereview.chromium.org/1559243002/diff/520001/src/IceELFSection.h#ne...
src/IceELFSection.h:371: // In ELF64, PC-relative relocations' addend need to
account for
On 2016/01/13 18:28:26, stichnot wrote:
> "addends" (for agreement)

Done.

https://codereview.chromium.org/1559243002/diff/520001/src/IceELFSection.h#ne...
src/IceELFSection.h:371: // In ELF64, PC-relative relocations' addend need to
account for
On 2016/01/13 18:28:26, stichnot wrote:
> reflow comment to 80-col

Done.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceInst.cpp#newcod...
src/IceInst.cpp:863: "align_to_end /* pad_to end */";
On 2016/01/13 18:28:26, stichnot wrote:
> Is the comment supposed to be "pad_to_end"? (two underscores)

Done.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.cpp
File src/IceInstX8664.cpp (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:239: const Ice::TargetLowering *TargetLowering, bool
LeaAddr) const {
On 2016/01/13 18:28:26, stichnot wrote:
> Suggestion: rename the parameter to IsLeaAddr to make the intent slightly
> clearer.

Done.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:243: const bool NeedSandboxing = Target->needSandboxing();
On 2016/01/13 18:28:26, stichnot wrote:
> Move this closer to the assert?

Done.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def
File src/IceInstX8664.def (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:33: //   zeroptr: This register is used as %rzp for NaCl
sandboxing.
On 2016/01/13 18:28:26, stichnot wrote:
> Sorry to do this, but could you also add the same new column to the x8632
file? 
> I'm trying to keep them consistent (even if a column is useless to some target
> and therefore all zeroes).

why? these are specific to each backend. For example, ARM tables are completely
different.

Having that column there is confusing. Someone reading the code might be tempted
to think there's a zeroptr for x8632.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:61: X(Reg_bpl,   5, "bpl",  Reg_rbp, 0,0,0,1,0, 1,0,0,0,0,
0, 0,0,0,1,0,         \
On 2016/01/13 18:28:26, stichnot wrote:
> The "preserved" bit changed to 0, I don't think that's right.

I had disabled rbp's (and friends') usage because they are handled specially in
x86-64 sandboxing. This has been reverted.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:71: X(Reg_r10l, 10, "r10b", Reg_r10, 1,0,0,0,0, 1,0,0,0,1,
0, 0,0,0,1,0,         \
On 2016/01/13 18:28:26, stichnot wrote:
> Yikes, nice catch here...

Acknowledged.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:112: X(Reg_rbp, 5,  "rbp", Reg_rbp, 0,0,0,1,0, 1,0,0,0,0,
0, 1,0,0,0,0,           \
On 2016/01/13 18:28:26, stichnot wrote:
> rbp is not a preserved register?  And it's not available for register
> allocation?

Acknowledged.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:145: X(Reg_ebp,   5, "ebp",  Reg_rbp, 0,0,0,1,0, 1,0,0,0,0,
0, 0,1,0,0,0,         \
On 2016/01/13 18:28:26, stichnot wrote:
> Same questions as rbp above.  (I'm starting to see a pattern here...)

Acknowledged.

https://codereview.chromium.org/1559243002/diff/520001/src/IceInstX8664.def#n...
src/IceInstX8664.def:178: X(Reg_bp,    5, "bp",   Reg_rbp, 0,0,0,1,0, 1,0,0,0,0,
0, 0,0,1,0,0,         \
On 2016/01/13 18:28:26, stichnot wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1559243002/diff/520001/src/IceLiveness.cpp
File src/IceLiveness.cpp (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceLiveness.cpp#ne...
src/IceLiveness.cpp:61: if (TmpNumGlobals != 0) {
On 2016/01/13 18:28:26, stichnot wrote:
> Did you mean to leave this here?  If so, it needs to be guarded by
> BuildDefs::dump() and some isVerbose() check.

Done.

https://codereview.chromium.org/1559243002/diff/520001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/1559243002/diff/520001/src/IceTypes.def#newco...
src/IceTypes.def:25: // TODO: Whoever adds AArch64 will need to set ABI e_flags.
On 2016/01/13 18:28:27, stichnot wrote:
> Since you touched this last :)
> Can you add a name to the TODO?

I thought about that, but who's supposed to be the TODO'er? I left a TODO(jpp),
but TODO(???) seems more appropriate.

https://codereview.chromium.org/1559243002/diff/520001/src/IceTypes.def#newco...
src/IceTypes.def:29: X(Target_X8664, "x86-64", true, EM_X86_64,  0)             
                 \
On 2016/01/13 18:28:26, stichnot wrote:
> fix beautiful column alignment

Done.

https://codereview.chromium.org/1559243002/diff/520001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/callindirect.pnacl.ll (right):

https://codereview.chromium.org/1559243002/diff/520001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/callindirect.pnacl.ll:70: ; X8664-LABEL: CallIndirect
On 2016/01/13 18:28:27, stichnot wrote:
> Why bother with these two RUN configs if all you're doing is checking whether
> the first function translates at all?

Because I was going to enable these for x86-64

[sehr, 2016-01-13 21:14:01]

LGTM.  Ultra-nits.

https://codereview.chromium.org/1559243002/diff/540001/src/IceAssemblerX86Bas...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1559243002/diff/540001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:167: emitInt32(-4);
Magic constant -4?  What's the relevance of that number?

https://codereview.chromium.org/1559243002/diff/560001/tests_lit/assembler/x8...
File tests_lit/assembler/x86/sandboxing.ll (right):

https://codereview.chromium.org/1559243002/diff/560001/tests_lit/assembler/x8...
tests_lit/assembler/x86/sandboxing.ll:340: 
Remove the ending blank line :-).

[John, 2016-01-13 21:40:25]

https://codereview.chromium.org/1559243002/diff/540001/src/IceAssemblerX86Bas...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1559243002/diff/540001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:167: emitInt32(-4);
On 2016/01/13 21:14:01, sehr wrote:
> Magic constant -4?  What's the relevance of that number.

-4 is usually emitted because, in x86-32, this value is the addend to the
relocation. In x86-64, this value does not really matter, so this was not
causing any problems.

It turns out that, for Absolute Relocations, this value should be zero.
Otherwise, the pushed value will be Label-4 -- which is clearly wrong.

https://codereview.chromium.org/1559243002/diff/560001/tests_lit/assembler/x8...
File tests_lit/assembler/x86/sandboxing.ll (right):

https://codereview.chromium.org/1559243002/diff/560001/tests_lit/assembler/x8...
tests_lit/assembler/x86/sandboxing.ll:340: 
On 2016/01/13 21:14:01, sehr wrote:
> Remove the ending blank line :-).

Done.

[Jim Stichnoth, 2016-01-14 00:09:52]

otherwise lgtm

https://codereview.chromium.org/1559243002/diff/580001/src/IceAssemblerX86Bas...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:164: intptr_t call_start = Buffer.getPosition();
CallStart

https://codereview.chromium.org/1559243002/diff/580001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:167: // In x86-32, the emitted value is an addend
to the relocation. Therefore,
Reflow to 80-col, I think

https://codereview.chromium.org/1559243002/diff/580001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:3332: void
AssemblerX86Base<TraitsType>::bind(Label *label) {
label==>Label while you're at it...

https://codereview.chromium.org/1559243002/diff/580001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceCfgNode.cpp#new...
src/IceCfgNode.cpp:1181: // after the instruction sequence starts starts at a
bundle boundary.
starts starts

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX8632.cpp#n...
src/IceInstX8632.cpp:263: const Ice::TargetLowering *TargetLowering, bool
/*LeaAddr*/) const {
IsLeaAddr

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX8664.cpp
File src/IceInstX8664.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:144: if (!Base && !Index) {
I prefer "Base == nullptr" over "!Base"
... though I'm generally OK eliding "!= nullptr"

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:2349: if (Traits::Is64Bit) {
I would just add this to the && conjunction below and avoid nested ifs.

https://codereview.chromium.org/1559243002/diff/580001/src/IceLiveness.cpp
File src/IceLiveness.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceLiveness.cpp#ne...
src/IceLiveness.cpp:58: if (IsFullInit) {
I would just revert this change completely in this CL.

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:230: // %rbp to be 'truncted' to 32-bit before
memory access.
truncated

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:300: // temporary. If an lea is goign to be
emitted, then eliding this movzx
going

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664Traits.h:548: // register talias table still needs to
be defined.
alias

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:364: /// x86-64 nacl sandbox.
NaCl

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:6281: // Not completely sure whether it's OK
to leave IsRebased unset when
reflow

https://codereview.chromium.org/1559243002/diff/580001/tests_lit/assembler/x8...
File tests_lit/assembler/x86/sandboxing.ll (right):

https://codereview.chromium.org/1559243002/diff/580001/tests_lit/assembler/x8...
tests_lit/assembler/x86/sandboxing.ll:273: ; X8664-LABEL:
bundle_lock_pad_to_end_padding_0
I suggest adding CHECK-LABEL lines just to ensure proper synchronization.

[John, 2016-01-14 17:18:22]

Description was changed from

==========
Suzero. X8664. NaCl Sandboxing.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

to

==========
Suzero. X8664. NaCl Sandboxing.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
R=sehr@chromium.org, stichnot@chromium.org

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

[John, 2016-01-14 17:18:23]

Committed patchset #31 (id:600001) manually as
56958cb33d3c1d045f2844408d825442d523f59f (presubmit successful).

[John, 2016-01-14 23:18:25]

https://codereview.chromium.org/1559243002/diff/580001/src/IceAssemblerX86Bas...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:164: intptr_t call_start = Buffer.getPosition();
On 2016/01/14 00:09:52, stichnot wrote:
> CallStart

Well, this is no longer needed, so I removed it.

https://codereview.chromium.org/1559243002/diff/580001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:167: // In x86-32, the emitted value is an addend
to the relocation. Therefore,
On 2016/01/14 00:09:52, stichnot wrote:
> Reflow to 80-col, I think

Done.

https://codereview.chromium.org/1559243002/diff/580001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:3332: void
AssemblerX86Base<TraitsType>::bind(Label *label) {
On 2016/01/14 00:09:52, stichnot wrote:
> label==>Label while you're at it...

Label is the type name. The code looks very confusing (to me) if I see Label
being used as something other than a type. I renamed it L.

<rant>thanks, llvm style guide</rant>

https://codereview.chromium.org/1559243002/diff/580001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceCfgNode.cpp#new...
src/IceCfgNode.cpp:1181: // after the instruction sequence starts starts at a
bundle boundary.
On 2016/01/14 00:09:52, stichnot wrote:
> starts starts

Done.

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX8632.cpp#n...
src/IceInstX8632.cpp:263: const Ice::TargetLowering *TargetLowering, bool
/*LeaAddr*/) const {
On 2016/01/14 00:09:52, stichnot wrote:
> IsLeaAddr

Done.

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX8664.cpp
File src/IceInstX8664.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX8664.cpp#n...
src/IceInstX8664.cpp:144: if (!Base && !Index) {
On 2016/01/14 00:09:52, stichnot wrote:
> I prefer "Base == nullptr" over "!Base"
> ... though I'm generally OK eliding "!= nullptr"

Call me a pascal programmer, but I am never OK with if (Ptr). A pointer is not a
boolean! :P

That said, this hideous idiom appeared here because of the previous condition --
my brain just inverted what was there.

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:2349: if (Traits::Is64Bit) {
On 2016/01/14 00:09:52, stichnot wrote:
> I would just add this to the && conjunction below and avoid nested ifs.

I thought about this, but I decided to go with this implementation because it
shows more clearly that the nested if is only applicable in x86-64. This is also
a somewhat common idiom throughout the code base (exception: boolean constant
initialization.)

https://codereview.chromium.org/1559243002/diff/580001/src/IceLiveness.cpp
File src/IceLiveness.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceLiveness.cpp#ne...
src/IceLiveness.cpp:58: if (IsFullInit) {
On 2016/01/14 00:09:52, stichnot wrote:
> I would just revert this change completely in this CL.

This was on the spirit of last afternoon's discussion about no having
single-line ifs without { }.

Done, though.

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:230: // %rbp to be 'truncted' to 32-bit before
memory access.
On 2016/01/14 00:09:52, stichnot wrote:
> truncated

(I meant to write trunc'ed.)

Done.

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:300: // temporary. If an lea is goign to be
emitted, then eliding this movzx
On 2016/01/14 00:09:52, stichnot wrote:
> going

Done.

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664Traits.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664Traits.h:548: // register talias table still needs to
be defined.
On 2016/01/14 00:09:52, stichnot wrote:
> alias

Done.

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:364: /// x86-64 nacl sandbox.
On 2016/01/14 00:09:52, stichnot wrote:
> NaCl

Done.

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1559243002/diff/580001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:6281: // Not completely sure whether it's OK
to leave IsRebased unset when
On 2016/01/14 00:09:52, stichnot wrote:
> reflow

Done.

https://codereview.chromium.org/1559243002/diff/580001/tests_lit/assembler/x8...
File tests_lit/assembler/x86/sandboxing.ll (right):

https://codereview.chromium.org/1559243002/diff/580001/tests_lit/assembler/x8...
tests_lit/assembler/x86/sandboxing.ll:273: ; X8664-LABEL:
bundle_lock_pad_to_end_padding_0
On 2016/01/14 00:09:52, stichnot wrote:
> I suggest adding CHECK-LABEL lines just to ensure proper synchronization.

Done.