Refactor llvm2ice so that Ice can be built while reading bitcode.

Refactor llvm2ice so that Ice can be built while reading bitcode.

BUG=None
R=stichnot@chromium.org

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

[Karl, 2014-06-24 19:53:22]

PTAL. Thanks.

[Jim Stichnoth, 2014-06-24 23:06:38]

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:1: //===- subzero/src/IceConverter.h - Converts LLVM to Ice
 ------*- C++ -*-===//
Fill to 80 chars :)
And the filename is wrong.
(note that .cpp files don't need the *- C++ -* stuff)

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h#newcode1
src/IceConverter.h:1: //===- subzero/src/IceConverter.h - Converts LLVM to Ice 
------*- C++ -*-===//
Space this out to 80 chars like the other similar lines. :)
Also, ICE not Ice.

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h#newco...
src/IceConverter.h:19: namespace llvm {
Does this need to be inside the llvm namespace?  I wouldn't expect it to be in
any namespace.

Maybe you wanted "using namespace llvm;" ?

https://codereview.chromium.org/350933002/diff/20001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/350933002/diff/20001/src/llvm2ice.cpp#newcode98
src/llvm2ice.cpp:98: "build-on-read", cl::desc("Build ice instruction when
reading bitcode"),
"Build ICE instructions when reading bitcode"

https://codereview.chromium.org/350933002/diff/20001/src/llvm2ice.cpp#newcode124
src/llvm2ice.cpp:124: Ice::VerboseMask VMask = Ice::IceV_None;
Can the code from here to the Ice::GlobalContext Ctx() line be moved to the
beginning of main()?  Then I think there could be just one test on BuildOnRead.

[Karl, 2014-06-25 16:02:06]

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:1: //===- subzero/src/IceConverter.h - Converts LLVM to Ice
 ------*- C++ -*-===//
On 2014/06/24 23:06:37, stichnot wrote:
> Fill to 80 chars :)
> And the filename is wrong.
> (note that .cpp files don't need the *- C++ -* stuff)

Done.

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:36: 
Enclosed following code in an unnamed namespace.

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h#newcode1
src/IceConverter.h:1: //===- subzero/src/IceConverter.h - Converts LLVM to Ice 
------*- C++ -*-===//
On 2014/06/24 23:06:37, stichnot wrote:
> Space this out to 80 chars like the other similar lines. :)
> Also, ICE not Ice.

Done.

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h#newco...
src/IceConverter.h:19: namespace llvm {
On 2014/06/24 23:06:37, stichnot wrote:
> Does this need to be inside the llvm namespace?  I wouldn't expect it to be in
> any namespace.
> 
> Maybe you wanted "using namespace llvm;" ?

First off, it is a bad idea to use "using namespace llvm" in a header file. This
class should be put into some namespace, so that it doesn't conflict with other
usages. I used "llvm" not knowing what space to put it in. I assume that it
shouldn't be in "Ice" either. Changing it to "Llvm2Ice", and removing
corresponding prefix on class name.

https://codereview.chromium.org/350933002/diff/20001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/350933002/diff/20001/src/llvm2ice.cpp#newcode98
src/llvm2ice.cpp:98: "build-on-read", cl::desc("Build ice instruction when
reading bitcode"),
On 2014/06/24 23:06:38, stichnot wrote:
> "Build ICE instructions when reading bitcode"

Done.

https://codereview.chromium.org/350933002/diff/20001/src/llvm2ice.cpp#newcode124
src/llvm2ice.cpp:124: Ice::VerboseMask VMask = Ice::IceV_None;
On 2014/06/24 23:06:38, stichnot wrote:
> Can the code from here to the Ice::GlobalContext Ctx() line be moved to the
> beginning of main()?  Then I think there could be just one test on
BuildOnRead.

Done.

[Jim Stichnoth, 2014-06-26 23:50:13]

LGTM with minor nits.

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h#newco...
src/IceConverter.h:19: namespace llvm {
On 2014/06/25 16:02:05, Karl wrote:
> On 2014/06/24 23:06:37, stichnot wrote:
> > Does this need to be inside the llvm namespace?  I wouldn't expect it to be
in
> > any namespace.
> > 
> > Maybe you wanted "using namespace llvm;" ?
> 
> First off, it is a bad idea to use "using namespace llvm" in a header file.
This
> class should be put into some namespace, so that it doesn't conflict with
other
> usages. I used "llvm" not knowing what space to put it in. I assume that it
> shouldn't be in "Ice" either. Changing it to "Llvm2Ice", and removing
> corresponding prefix on class name.

Yeah, you're right about "using namespace" and header files.

I would just use the Ice namespace, especially since this is a subzero/src/Ice*
file.

https://codereview.chromium.org/350933002/diff/40001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/350933002/diff/40001/src/IceConverter.h#newco...
src/IceConverter.h:19: namespace llvm {
Could we just declare like this:
  class llvm::Module;
?

https://codereview.chromium.org/350933002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/350933002/diff/40001/src/llvm2ice.cpp#newcode131
src/llvm2ice.cpp:131: Module *Mod = 0;
Remove this line and declare/define Mod 2 lines down.

[Karl, 2014-06-27 16:15:22]

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/350933002/diff/20001/src/IceConverter.h#newco...
src/IceConverter.h:19: namespace llvm {
On 2014/06/26 23:50:12, stichnot wrote:
> On 2014/06/25 16:02:05, Karl wrote:
> > On 2014/06/24 23:06:37, stichnot wrote:
> > > Does this need to be inside the llvm namespace?  I wouldn't expect it to
be
> in
> > > any namespace.
> > > 
> > > Maybe you wanted "using namespace llvm;" ?
> > 
> > First off, it is a bad idea to use "using namespace llvm" in a header file.
> This
> > class should be put into some namespace, so that it doesn't conflict with
> other
> > usages. I used "llvm" not knowing what space to put it in. I assume that it
> > shouldn't be in "Ice" either. Changing it to "Llvm2Ice", and removing
> > corresponding prefix on class name.
> 
> Yeah, you're right about "using namespace" and header files.
> 
> I would just use the Ice namespace, especially since this is a
subzero/src/Ice*
> file.

Done.

https://codereview.chromium.org/350933002/diff/40001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/350933002/diff/40001/src/IceConverter.h#newco...
src/IceConverter.h:19: namespace llvm {
On 2014/06/26 23:50:12, stichnot wrote:
> Could we just declare like this:
>   class llvm::Module;
> ?

The problem is that doesn't work (you can't use a namespace that hasn't been
referred to in the file). When I compile it I get:

   error: ‘Module’ in namespace ‘llvm’ does not name a type

That's why I put it inside a namespace.

https://codereview.chromium.org/350933002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/350933002/diff/40001/src/llvm2ice.cpp#newcode131
src/llvm2ice.cpp:131: Module *Mod = 0;
On 2014/06/26 23:50:12, stichnot wrote:
> Remove this line and declare/define Mod 2 lines down.

Done.

[Karl, 2014-06-27 16:15:37]

Committed patchset #4 manually as re1e013c (presubmit successful).