Extracts an TargetX86Base target which will be used as the common X86{32,64} implementation.

Extracts an TargetX86Base target which will be used as the common X86{32,64} implementation.

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=7e93c62d7e223b7fd9e6e0889e4b70b635589282

[John, 2015-06-22 21:10:00]

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

[John, 2015-06-22 21:10:01]

There will be more CLs for refactoring this code, but it isn't clear to me yet
what they are going to be like. I'll modify the code as needed.

[Jim Stichnoth, 2015-06-22 21:52:02]

Some surface comments while I absorb the rest.

https://codereview.chromium.org/1202533003/diff/1/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1202533003/diff/1/Makefile.standalone#newcode252
Makefile.standalone:252: $(CXX) -ferror-limit=999999 -c $(CXXFLAGS) $< -o $@
Not sure I want to live on this planet any more.

https://codereview.chromium.org/1202533003/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1202533003/diff/1/src/IceInst.h#newcode19
src/IceInst.h:19: #include "IceCfg.h"
Is this include actually needed?

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1: //===-
subzero/src/IceTargetLoweringX86BaseImpl.h - x86 lowering ----------===//
add the "-*- C++ -*-" stuff

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:15: 
Should there be an include guard?

[John, 2015-06-22 22:09:24]

https://codereview.chromium.org/1202533003/diff/1/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1202533003/diff/1/Makefile.standalone#newcode252
Makefile.standalone:252: $(CXX) -ferror-limit=999999 -c $(CXXFLAGS) $< -o $@
On 2015/06/22 21:52:01, stichnot wrote:
> Not sure I want to live on this planet any more.

<rant>I hate this option! I want to compiler to spit everything out, especially
when I am using templates, as sometimes the error lives at the bottom of the
error report.</rant>

Done.

https://codereview.chromium.org/1202533003/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1202533003/diff/1/src/IceInst.h#newcode19
src/IceInst.h:19: #include "IceCfg.h"
On 2015/06/22 21:52:02, stichnot wrote:
> Is this include actually needed?

Yes, it is. This header uses

Func->...

which means Cfg needs to be defined (not just declared.)

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1: //===-
subzero/src/IceTargetLoweringX86BaseImpl.h - x86 lowering ----------===//
On 2015/06/22 21:52:02, stichnot wrote:
> add the "-*- C++ -*-" stuff

Done, but why? Also, this C++ thingy adds no real value other than requiring
people to write something that should be clear if you're reading the source
code. It also limits file source name.

Also, notice that the comment is *- C++ -*-, and not -*- C++ -*- because the
latter does not fit.

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:15: 
On 2015/06/22 21:52:02, stichnot wrote:
> Should there be an include guard?

It doesn't hurt, but Impl files should not be included by anything other than
the files that declare the stuff they implement.

[Jim Stichnoth, 2015-06-22 23:04:05]

https://codereview.chromium.org/1202533003/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1202533003/diff/1/src/IceInst.h#newcode19
src/IceInst.h:19: #include "IceCfg.h"
On 2015/06/22 22:09:23, John wrote:
> On 2015/06/22 21:52:02, stichnot wrote:
> > Is this include actually needed?
> 
> Yes, it is. This header uses
> 
> Func->...
> 
> which means Cfg needs to be defined (not just declared.)

I see.  Looks like we've just gotten lucky so far.

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1: //===-
subzero/src/IceTargetLoweringX86BaseImpl.h - x86 lowering ----------===//
On 2015/06/22 22:09:23, John wrote:
> On 2015/06/22 21:52:02, stichnot wrote:
> > add the "-*- C++ -*-" stuff
> 
> Done, but why? Also, this C++ thingy adds no real value other than requiring
> people to write something that should be clear if you're reading the source
> code. It also limits file source name.
> 
> Also, notice that the comment is *- C++ -*-, and not -*- C++ -*- because the
> latter does not fit.

This is an emacs thing, so that it invokes c++-mode instead of c-mode.

Unfortunately, it does need the full string, and we emacs users really depend on
this.  It looks like LLVM steals from the '===' in these cases.

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:15: 
On 2015/06/22 22:09:23, John wrote:
> On 2015/06/22 21:52:02, stichnot wrote:
> > Should there be an include guard?
> 
> It doesn't hurt, but Impl files should not be included by anything other than
> the files that declare the stuff they implement.

OK, I see.

https://codereview.chromium.org/1202533003/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1202533003/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:5504: 
git (or someone) warns about this "trailing whitespace". :)

[John, 2015-06-22 23:09:55]

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1202533003/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1: //===-
subzero/src/IceTargetLoweringX86BaseImpl.h - x86 lowering ----------===//
On 2015/06/22 23:04:05, stichnot wrote:
> On 2015/06/22 22:09:23, John wrote:
> > On 2015/06/22 21:52:02, stichnot wrote:
> > > add the "-*- C++ -*-" stuff
> > 
> > Done, but why? Also, this C++ thingy adds no real value other than requiring
> > people to write something that should be clear if you're reading the source
> > code. It also limits file source name.
> > 
> > Also, notice that the comment is *- C++ -*-, and not -*- C++ -*- because the
> > latter does not fit.
> 
> This is an emacs thing, so that it invokes c++-mode instead of c-mode.
Use vim instead? :)

Seriously, though, it is puzzling that

(require 'cc-mode)

does not work.

> 
> Unfortunately, it does need the full string, and we emacs users really depend
on
> this.  It looks like LLVM steals from the '===' in these cases.

https://codereview.chromium.org/1202533003/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1202533003/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:5504: 
On 2015/06/22 23:04:05, stichnot wrote:
> git (or someone) warns about this "trailing whitespace". :)

Done.

I usually add the extra newline so that

cat <file>

does not end with

...
#endif // SUBZERO_SRC_ICETARGETLOWERINGX86BASEIMPL_Hjpp@jpp...

[Jim Stichnoth, 2015-06-23 17:24:34]

LGTM.

I poked around with the patch, and its usability seems similar to the original
structure.  I was concerned about putting all the code in a .h file, with
respect to code size, but that doesn't seem to be an issue.

https://codereview.chromium.org/1202533003/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/1202533003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:27: class Cfg;
These are already forward-declared in IceDefs.h.

[John, 2015-06-23 17:52:51]

https://codereview.chromium.org/1202533003/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/1202533003/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:27: class Cfg;
On 2015/06/23 17:24:33, stichnot wrote:
> These are already forward-declared in IceDefs.h.

IceDefs..... :(
Done.

[John, 2015-06-23 17:59:03]

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

[Jim Stichnoth, 2015-06-23 18:07:15]

BTW, IceTargetLoweringX8664.h introduces some new warnings (unused parameters),
hopefully that will get fixed in the next CL.