Subzero. Native 64-bit int arithmetic on x86-64.

Subzero. Native 64-bit int arithmetic on x86-64.

This CL modifies the x86 instruction selection template to allow native
64-bit GPR support. It also enables x86-64 crosstests.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4077
R=stichnot@chromium.org

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

[John, 2015-08-06 17:57:40]

Patchset #4 (id:60001) has been deleted

[John, 2015-08-07 22:35:57]

jpp@chromium.org changed reviewers:
+ jfb@chromium.org, jvoung@chromium.org, stichnot@chromium.org

[John, 2015-08-07 22:35:58]

Here it is, in all of its g[l]ory details, the initial TargetX8664. I already
got the crosstests to run (all but test_global which llc can't build.)

There's still some work to be done here. Specifically:
1) lit tests; and
2) assembler tests; (I modified the x86 assembler but did not add any tests for
the modification -- and did not fix whatever the changes broke.)

This is pretty much the meaty part of this CL (which is surprisingly small.) I
decided to send the CL for review to speed things up a bit.

[Jim Stichnoth, 2015-08-10 16:08:05]

Preliminary comments, I still need to look closely at
IceTargetLoweringX86BaseImpl.h and a couple others.

https://codereview.chromium.org/1273153002/diff/350001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1273153002/diff/350001/Makefile.standalone#ne...
Makefile.standalone:358: -e x8632,sandbox,sse4.1,Om1 \
Why disable x86-32 sandbox tests?

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
File crosstest/stack_hack.x8664.c (right):

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
crosstest/stack_hack.x8664.c:1: //===- subzero/crosstest/stack_hack.x8664.c.h -
X8664 stack hack ----------===//
no .h

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
crosstest/stack_hack.x8664.c:35: const uint32 PageBoundary = 4 << 20; // 4 MB.
May want to "(void)PageBoundary;" in case we ever ramp up the warning/error
levels.

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
crosstest/stack_hack.x8664.c:45: void *Foo = xStackStart(StackEnd, Size);
Foo?

https://codereview.chromium.org/1273153002/diff/350001/crosstest/xdefs.h
File crosstest/xdefs.h (right):

https://codereview.chromium.org/1273153002/diff/350001/crosstest/xdefs.h#newc...
crosstest/xdefs.h:15: #ifndef XDEFS_H_
Probably better to use SUBZERO_CROSSTEST_XDEFS_H.  (I realize other include
files here have the same problem...)

https://codereview.chromium.org/1273153002/diff/350001/crosstest/xdefs.h#newc...
crosstest/xdefs.h:27: // x86_64_64 even with and ILP32 model. This hack
allocates a new stack for
x86_64

also, s/and/an/

BTW, I'm amazed that only the stack needs to be hacked...

https://codereview.chromium.org/1273153002/diff/350001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1273153002/diff/350001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:2257: return llvm::dyn_cast<ConstantInteger32>(Op) !=
nullptr ||
Use isa<> instead of dyn_cast<> .

https://codereview.chromium.org/1273153002/diff/350001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:2348: const auto *C32 =
llvm::dyn_cast<ConstantInteger32>(Src);
Can probably use llvm::cast<> which would do the assert for you.

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:330: int32_t RegNum = -1;
I think you should be able to use Variable::NoRegister instead of -1.  IIRC, the
time I needed to use -1 is in the register allocator because of the
llvm::BitVector::find_first() semantics.

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:86: // TODO(jpp): this needs to round to
multiples of 8 bytes in x86-64.
remove TODO?

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:117: "Hey, yo! This is x86-64. Watcha doin'?
(split64)");
Made me laugh, but... more professional strings for release?

[Jim Stichnoth, 2015-08-10 19:39:20]

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:124: return PK_Icmp32;
Probably need to rename PK_Icmp32 to something reflecting a one-operation
compare (versus the 2-3 compares from the i64->i32 lowering).  Maybe leave a
TODO somewhere

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2086: TargetString =
isFloat32Asserting32Or64(SrcType) ? H_fptoui_f32_i64
Does it make sense to combine the first and third branches of this if/else/else
construct, since they set TargetString to the same value?

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2263: Operand *Src0R = legalize(Src0,
Legal_Reg);
Maybe this?
  Variable *Src0R = legalizeToReg(Src0);

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2306: Operand *Src0R = legalize(Src0,
Legal_Reg | Legal_Mem);
Name this Src0RM

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2951: // Make sure the atomic load isn't
elided when unused, by adding a
hmm, why is a linebreak added?

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:3082: // is dangerous because there's a
chance the copiler will optimize this
compiler

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:3289: // There might be phi assignments right
before the compare+branch, since
reformat

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:3559: // like (31 - N) for N <= 31, and
converts 63 to 32 (for the all-zeros
reformat?

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:3632: // possible
More weird linebreak stuff

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4828: if (auto *C =
llvm::dyn_cast<ConstantInteger64>(Const)) {
Use isa<> instead of dyn_cast<>.

[John, 2015-08-10 20:41:17]

https://codereview.chromium.org/1273153002/diff/350001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1273153002/diff/350001/Makefile.standalone#ne...
Makefile.standalone:358: -e x8632,sandbox,sse4.1,Om1 \
On 2015/08/10 16:08:04, stichnot wrote:
> Why disable x86-32 sandbox tests?

typo. done.

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
File crosstest/stack_hack.x8664.c (right):

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
crosstest/stack_hack.x8664.c:1: //===- subzero/crosstest/stack_hack.x8664.c.h -
X8664 stack hack ----------===//
On 2015/08/10 16:08:04, stichnot wrote:
> no .h

Done.

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
crosstest/stack_hack.x8664.c:35: const uint32 PageBoundary = 4 << 20; // 4 MB.
On 2015/08/10 16:08:04, stichnot wrote:
> May want to "(void)PageBoundary;" in case we ever ramp up the warning/error
> levels.

Done.

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
crosstest/stack_hack.x8664.c:45: void *Foo = xStackStart(StackEnd, Size);
On 2015/08/10 16:08:04, stichnot wrote:
> Foo?

Bar instead?

https://codereview.chromium.org/1273153002/diff/350001/crosstest/xdefs.h
File crosstest/xdefs.h (right):

https://codereview.chromium.org/1273153002/diff/350001/crosstest/xdefs.h#newc...
crosstest/xdefs.h:15: #ifndef XDEFS_H_
On 2015/08/10 16:08:04, stichnot wrote:
> Probably better to use SUBZERO_CROSSTEST_XDEFS_H.  (I realize other include
> files here have the same problem...)

Done.

https://codereview.chromium.org/1273153002/diff/350001/crosstest/xdefs.h#newc...
crosstest/xdefs.h:27: // x86_64_64 even with and ILP32 model. This hack
allocates a new stack for
On 2015/08/10 16:08:04, stichnot wrote:
> x86_64
> 
> also, s/and/an/
> 
> BTW, I'm amazed that only the stack needs to be hacked...

X86_64_64 was a failed attempt to mean x86-64 LP64.

https://codereview.chromium.org/1273153002/diff/350001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1273153002/diff/350001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:2257: return llvm::dyn_cast<ConstantInteger32>(Op) !=
nullptr ||
On 2015/08/10 16:08:04, stichnot wrote:
> Use isa<> instead of dyn_cast<> .

Done.

https://codereview.chromium.org/1273153002/diff/350001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:2348: const auto *C32 =
llvm::dyn_cast<ConstantInteger32>(Src);
On 2015/08/10 16:08:04, stichnot wrote:
> Can probably use llvm::cast<> which would do the assert for you.

Done.

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:330: int32_t RegNum = -1;
On 2015/08/10 16:08:05, stichnot wrote:
> I think you should be able to use Variable::NoRegister instead of -1.  IIRC,
the
> time I needed to use -1 is in the register allocator because of the
> llvm::BitVector::find_first() semantics.

Done.

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:86: // TODO(jpp): this needs to round to
multiples of 8 bytes in x86-64.
On 2015/08/10 16:08:05, stichnot wrote:
> remove TODO?

Done.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:117: "Hey, yo! This is x86-64. Watcha doin'?
(split64)");
On 2015/08/10 16:08:05, stichnot wrote:
> Made me laugh, but... more professional strings for release?

:( but Google is known for these *Googley** strings. E.g.

Bad, bad server. No doughnut for you!
He's dead, Jim.
(The broken robot)

I'd rather keep these non-professional-googley strings unless you **REALLY**
oppose it. Let's keep google googley!

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:124: return PK_Icmp32;
On 2015/08/10 19:39:20, stichnot wrote:
> Probably need to rename PK_Icmp32 to something reflecting a one-operation
> compare (versus the 2-3 compares from the i64->i32 lowering).  Maybe leave a
> TODO somewhere

TODO'ed.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2086: TargetString =
isFloat32Asserting32Or64(SrcType) ? H_fptoui_f32_i64
On 2015/08/10 19:39:20, stichnot wrote:
> Does it make sense to combine the first and third branches of this
if/else/else
> construct, since they set TargetString to the same value?

I would rather not mix the cases. This is more explicit about what's really
happening (i.e., different actions for x64-32 v x32-64.)

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2263: Operand *Src0R = legalize(Src0,
Legal_Reg);
On 2015/08/10 19:39:20, stichnot wrote:
> Maybe this?
>   Variable *Src0R = legalizeToReg(Src0);

Done.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2306: Operand *Src0R = legalize(Src0,
Legal_Reg | Legal_Mem);
On 2015/08/10 19:39:20, stichnot wrote:
> Name this Src0RM

Done.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2951: // Make sure the atomic load isn't
elided when unused, by adding a
On 2015/08/10 19:39:19, stichnot wrote:
> hmm, why is a linebreak added?

make format.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:3082: // is dangerous because there's a
chance the copiler will optimize this
On 2015/08/10 19:39:20, stichnot wrote:
> compiler

Done.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:3289: // There might be phi assignments right
before the compare+branch, since
On 2015/08/10 19:39:20, stichnot wrote:
> reformat

Done.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:3559: // like (31 - N) for N <= 31, and
converts 63 to 32 (for the all-zeros
On 2015/08/10 19:39:20, stichnot wrote:
> reformat?

Done.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:3632: // possible
On 2015/08/10 19:39:20, stichnot wrote:
> More weird linebreak stuff

Done.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4828: if (auto *C =
llvm::dyn_cast<ConstantInteger64>(Const)) {
On 2015/08/10 19:39:20, stichnot wrote:
> Use isa<> instead of dyn_cast<>.

Is there any rule for isa v. dyn_cast? In this case I am using C below (although
arguably I could just use const.) In this case, I would argue dyn_cast will not
be worse than isa. Am I missing something?

[Jim Stichnoth, 2015-08-11 16:01:37]

otherwise lgtm.

Be sure to consult with Jan on the ELF relocation types, if you haven't already.

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
File crosstest/stack_hack.x8664.c (right):

https://codereview.chromium.org/1273153002/diff/350001/crosstest/stack_hack.x...
crosstest/stack_hack.x8664.c:45: void *Foo = xStackStart(StackEnd, Size);
On 2015/08/10 20:41:16, John wrote:
> On 2015/08/10 16:08:04, stichnot wrote:
> > Foo?
> 
> Bar instead?

Made me look, well played sir.

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:117: "Hey, yo! This is x86-64. Watcha doin'?
(split64)");
On 2015/08/10 20:41:17, John wrote:
> On 2015/08/10 16:08:05, stichnot wrote:
> > Made me laugh, but... more professional strings for release?
> 
> :( but Google is known for these *Googley** strings. E.g.
> 
> Bad, bad server. No doughnut for you!
> He's dead, Jim.
> (The broken robot)
> 
> I'd rather keep these non-professional-googley strings unless you **REALLY**
> oppose it. Let's keep google googley!

No **real** opposition.

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:124: return PK_Icmp32;
On 2015/08/10 20:41:17, John wrote:
> On 2015/08/10 19:39:20, stichnot wrote:
> > Probably need to rename PK_Icmp32 to something reflecting a one-operation
> > compare (versus the 2-3 compares from the i64->i32 lowering).  Maybe leave a
> > TODO somewhere
> 
> TODO'ed.

TODO'ed, dude?  TODONE.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2951: // Make sure the atomic load isn't
elided when unused, by adding a
On 2015/08/10 20:41:17, John wrote:
> On 2015/08/10 19:39:19, stichnot wrote:
> > hmm, why is a linebreak added?
> 
> make format.

Oh, so something like, you added an outer layer of {}, ran make format, then
removed the {} layer?  (I ask so that I can avoid it happening to me...)

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4828: if (auto *C =
llvm::dyn_cast<ConstantInteger64>(Const)) {
On 2015/08/10 20:41:17, John wrote:
> On 2015/08/10 19:39:20, stichnot wrote:
> > Use isa<> instead of dyn_cast<>.
> 
> Is there any rule for isa v. dyn_cast? In this case I am using C below
(although
> arguably I could just use const.) In this case, I would argue dyn_cast will
not
> be worse than isa. Am I missing something?

You're probably right about equivalent code generation being likely.

I didn't find anything explicitly about this in the LLVM documentation.

My feeling is that isa<> makes it more clear that you are just verifying a type,
whereas with cast<> or dyn_cast<>, I assume you will be making use of some
subtype-specific field or method.  So isa<> makes it easier for me to reason
about the code in the block (small as it might be).

https://codereview.chromium.org/1273153002/diff/280043/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1273153002/diff/280043/Makefile.standalone#ne...
Makefile.standalone:358: -e x8664,sandbox,sse4.1,Om1 \
This -e actually has no effect, since none of the -i options would have included
x8664+sandbox.  I guess as it stands, it's mostly documentation of what else is
to come?  May be better to express that as a TODO comment.

https://codereview.chromium.org/1273153002/diff/280043/crosstest/xdefs.h
File crosstest/xdefs.h (right):

https://codereview.chromium.org/1273153002/diff/280043/crosstest/xdefs.h#newc...
crosstest/xdefs.h:15: #ifndef SUBZERO_CROSSTESTS_XDEFS_H_
s/CROSSTESTS/CROSSTEST/

https://codereview.chromium.org/1273153002/diff/280043/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1273153002/diff/280043/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:302: template <typename T = Traits>
This pattern:
  template <typename T = Traits>
  typename std::enable_if<T::Is64Bit, XXX>::type
is long and repetitive, what do you think about macro-izing it?

https://codereview.chromium.org/1273153002/diff/280043/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1273153002/diff/280043/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1395: Str << "\tcdto";
This hasn't been done everywhere, but we've started trying to separate the
leading "\t" out of the string (e.g. Str << "\t" << "cltd") to make these
opcodes easier to "git grep -w".

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

https://codereview.chromium.org/1273153002/diff/280043/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:86: return (typeWidthInBytes(Ty) +
WordSizeInBytes - 1) &
/lazyme wonders if there's something in Ice::Utils to apply the alignment

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

https://codereview.chromium.org/1273153002/diff/280043/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4016: // A Load instruction can be treated
the same as an Assign instruction,
formatting

https://codereview.chromium.org/1273153002/diff/280043/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4658: // later
formatting

https://codereview.chromium.org/1273153002/diff/280043/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5153: // use-def
formatting

[John, 2015-08-12 19:27:55]

This CL is already pretty large. I want to avoid another painful merge (for me,
at least :)) at all costs, so I will land this.

This patch was tested with:
make CHECK_X86_ASM=1 DEBUG=1 check
make MINIMAL=1 check
make DEBUG=1
pydir/szbuild_spec2k.py --force -Om1 --filetype asm
(spec run_all.sh train)
pydir/szbuild_spec2k.py --force -Om1 --filetype obj
(spec run_all.sh train)
pydir/szbuild_spec2k.py --force -O2 --filetype asm
(spec run_all.sh train)
pydir/szbuild_spec2k.py --force -O2 --filetype obj
(spec run_all.sh train)
make MINIMAL=1
pydir/szbuild_spec2k.py --force -Om1 --filetype obj
(spec run_all.sh train)
pydir/szbuild_spec2k.py --force -O2 --filetype obj
(spec run_all.sh train)

NOTE: still no x86-64 xtests -- coming up shortly

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

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:2951: // Make sure the atomic load isn't
elided when unused, by adding a
On 2015/08/11 16:01:36, stichnot wrote:
> On 2015/08/10 20:41:17, John wrote:
> > On 2015/08/10 19:39:19, stichnot wrote:
> > > hmm, why is a linebreak added?
> > 
> > make format.
> 
> Oh, so something like, you added an outer layer of {}, ran make format, then
> removed the {} layer?  (I ask so that I can avoid it happening to me...)

Probably. I find annoying that clang format will break lines but will not merge
them back if at all possible.

https://codereview.chromium.org/1273153002/diff/350001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4828: if (auto *C =
llvm::dyn_cast<ConstantInteger64>(Const)) {
On 2015/08/11 16:01:36, stichnot wrote:
> On 2015/08/10 20:41:17, John wrote:
> > On 2015/08/10 19:39:20, stichnot wrote:
> > > Use isa<> instead of dyn_cast<>.
> > 
> > Is there any rule for isa v. dyn_cast? In this case I am using C below
> (although
> > arguably I could just use const.) In this case, I would argue dyn_cast will
> not
> > be worse than isa. Am I missing something?
> 
> You're probably right about equivalent code generation being likely.
> 
> I didn't find anything explicitly about this in the LLVM documentation.
> 
> My feeling is that isa<> makes it more clear that you are just verifying a
type,
> whereas with cast<> or dyn_cast<>, I assume you will be making use of some
> subtype-specific field or method.  So isa<> makes it easier for me to reason
> about the code in the block (small as it might be).

Fair enough. Done.

https://codereview.chromium.org/1273153002/diff/280043/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1273153002/diff/280043/Makefile.standalone#ne...
Makefile.standalone:358: -e x8664,sandbox,sse4.1,Om1 \
On 2015/08/11 16:01:37, stichnot wrote:
> This -e actually has no effect, since none of the -i options would have
included
> x8664+sandbox.  I guess as it stands, it's mostly documentation of what else
is
> to come?  May be better to express that as a TODO comment.

Done.

https://codereview.chromium.org/1273153002/diff/280043/crosstest/xdefs.h
File crosstest/xdefs.h (right):

https://codereview.chromium.org/1273153002/diff/280043/crosstest/xdefs.h#newc...
crosstest/xdefs.h:15: #ifndef SUBZERO_CROSSTESTS_XDEFS_H_
On 2015/08/11 16:01:37, stichnot wrote:
> s/CROSSTESTS/CROSSTEST/

Done.

https://codereview.chromium.org/1273153002/diff/280043/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1273153002/diff/280043/src/IceAssemblerX86Bas...
src/IceAssemblerX86Base.h:302: template <typename T = Traits>
On 2015/08/11 16:01:37, stichnot wrote:
> This pattern:
>   template <typename T = Traits>
>   typename std::enable_if<T::Is64Bit, XXX>::type
> is long and repetitive, what do you think about macro-izing it?

I personally dislike macros. It may not seem so given the amount of macros I
write, but I find them hard to read and error-prone. std::enable_if is idiomatic
and clear to anyone familiar with C++. True, it is verbose, but I take
verbose/boring over macros any time of the day. With that being said...

There are other ways to use std::enable_if that are less verbose and avoid using
macros altogether. I can address this issue in a separate CL.

https://codereview.chromium.org/1273153002/diff/280043/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1273153002/diff/280043/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1395: Str << "\tcdto";
On 2015/08/11 16:01:37, stichnot wrote:
> This hasn't been done everywhere, but we've started trying to separate the
> leading "\t" out of the string (e.g. Str << "\t" << "cltd") to make these
> opcodes easier to "git grep -w".

Done, but just raising a point...

what about the instructions where we have:

"inst" << type_suffix

?? Luckily there are not that many of these. :)

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

https://codereview.chromium.org/1273153002/diff/280043/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:86: return (typeWidthInBytes(Ty) +
WordSizeInBytes - 1) &
On 2015/08/11 16:01:37, stichnot wrote:
> /lazyme wonders if there's something in Ice::Utils to apply the alignment

Done.

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

https://codereview.chromium.org/1273153002/diff/280043/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4016: // A Load instruction can be treated
the same as an Assign instruction,
On 2015/08/11 16:01:37, stichnot wrote:
> formatting

Done.

https://codereview.chromium.org/1273153002/diff/280043/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:4658: // later
On 2015/08/11 16:01:37, stichnot wrote:
> formatting

Done.

https://codereview.chromium.org/1273153002/diff/280043/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5153: // use-def
On 2015/08/11 16:01:37, stichnot wrote:
> formatting

Done.

[Jim Stichnoth, 2015-08-12 19:37:47]

https://codereview.chromium.org/1273153002/diff/280043/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1273153002/diff/280043/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1395: Str << "\tcdto";
On 2015/08/12 19:27:55, John wrote:
> On 2015/08/11 16:01:37, stichnot wrote:
> > This hasn't been done everywhere, but we've started trying to separate the
> > leading "\t" out of the string (e.g. Str << "\t" << "cltd") to make these
> > opcodes easier to "git grep -w".
> 
> Done, but just raising a point...
> 
> what about the instructions where we have:
> 
> "inst" << type_suffix
> 
> ?? Luckily there are not that many of these. :)

In those cases, my preference is to tag the complete name in a comment (not that
I necessarily did any of that in this code base, but still).

[John, 2015-08-12 19:37:58]

Committed patchset #22 (id:420001) manually as
1d235425dab1f3dd059973fc53f1b1d5879469e3 (tree was closed).