Introduce the notion of function addresses in Subzero.

Introduce the notion of function addresses in Subzero.

Introduces the notion of a function address, to replace using LLVM
IR's Function class. Modifies Ice converter, and Subzero's bitcode
reader, to build function addresses.

BUG=None
R=jvoung@chromium.org, stichnot@chromium.org

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

[Karl, 2014-10-09 22:13:45]

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

[jvoung (off chromium), 2014-10-10 01:47:29]

some quick scan comments

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:59: public:
already in the public: section?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:60: Ice::Converter &getConverter() { return Converter; }
const ?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:693: const auto GV = dyn_cast<GlobalVariable>(I);
cast<> is a checked cast and would help assert(GV); ?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:704: assert(llvm::isa<Ice::GlobalVariable>(Addr));
sometimes llvm:: is used, but sometimes not?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:705: auto VarAddr = llvm::cast<Ice::GlobalVariable>(Addr);
cast<> already asserts isa<>, so the earlier assert for isa<> is redundant.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:761: if (Addr == NULL) {
nullptr

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:806: for (llvm::Function &Func : *Mod) {
could this be const ?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:824: const auto GV = dyn_cast<llvm::GlobalVariable>(I);
could just cast<> to assert that GV is really a GlobalVariable (instead of the
assert right after this?)

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:824: const auto GV = dyn_cast<llvm::GlobalVariable>(I);
In some places plain "GlobalVariable" is used for "llvm::GlobalVariable", while
only "Ice::GlobalVariable" has the "Ice::". It might be better to just have
"llvm::GlobalVariable" like this instance? WDYT?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:831: Vbl->setIsInternal(!GV->isExternallyInitialized());
Was there a commandline flag to check?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.h#newc...
src/IceConverter.h:34: ~Converter() {}
override?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.h#newc...
src/IceConverter.h:40: llvm::Module *getModule() { return Mod; }
method is "const"?

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:404: GlobalVariable *Vbl = new GlobalVariable();
Most abbreviations of "Variable" are "Var" in the other code.

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:108: /// Models a global address, and its initializers.
nit: How about "global variable address" otherwise, it looks like the
"GlobalAddress" class?

https://codereview.chromium.org/641193002/diff/150001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceTargetLowering.h...
src/IceTargetLowering.h:252: virtual void lower(const GlobalVariable &Vbl, bool
DisableTranslation) = 0;
nit: How about "Var" instead of "Vbl"?

https://codereview.chromium.org/641193002/diff/150001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:266: const FunctionListType &getFunctionList() { return
FunctionList; }
const ?

[Jim Stichnoth, 2014-10-10 13:15:01]

Initial comments.

https://codereview.chromium.org/641193002/diff/150001/crosstest/test_global_m...
File crosstest/test_global_main.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/crosstest/test_global_m...
crosstest/test_global_main.cpp:26: int ExternName1 = 36363;
The code referencing this is still #if'd out in test_global.cpp.  Should it be
re-enabled, or should these definitions wait for a later CL?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:37: using namespace llvm;
A number of "llvm::" prefixes crept in, should they be removed?

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:24: #include "IceClFlags.h"
Is this new include necessary?

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.cpp
File src/IceGlobalInits.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.cpp#...
src/IceGlobalInits.cpp:53: }
// end of anonymous namespace

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.cpp#...
src/IceGlobalInits.cpp:186: }
// end of namespace Ice

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:69: // it's calling conventions, and its linkage.
its

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:70: class Function : public GlobalAddress {
Could we use a different name than "Function" for this?  When I read it, it
makes me think of a fully defined function, like what Cfg captures.

https://codereview.chromium.org/641193002/diff/150001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:427: // Reports that there is no function address for
ID. Returns
fill-paragraph?

https://codereview.chromium.org/641193002/diff/150001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/globalrelocs.ll (right):

https://codereview.chromium.org/641193002/diff/150001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/globalrelocs.ll:128: ; CHECK:	.long	ptr + -1
Maybe the emit code could make this a little prettier? :)

[Karl, 2014-10-10 20:17:31]

https://codereview.chromium.org/641193002/diff/150001/crosstest/test_global_m...
File crosstest/test_global_main.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/crosstest/test_global_m...
crosstest/test_global_main.cpp:26: int ExternName1 = 36363;
On 2014/10/10 13:15:00, stichnot wrote:
> The code referencing this is still #if'd out in test_global.cpp.  Should it be
> re-enabled, or should these definitions wait for a later CL?

This doesn't work yet. I don't fully understand why. However, where I would
expect code generated from lowering constant initializers, I instead see code.
This implies that the relocation addresses in the initializer are being
converted to a ConstantRelocatable.

The problem is that I haven't yet changed a ConstantRelocatable to be based on a
GlobalAddress. Rather, it just copies the name to a local instance. Hence, it no
longer has the context to decide when a global name should be mangled (for cross
tests on relocations). This will be fixed in a later CL.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:37: using namespace llvm;
On 2014/10/10 13:15:00, stichnot wrote:
> A number of "llvm::" prefixes crept in, should they be removed?

There are several comments around the topic of namespace prefixes, each having
(somewhat conflicting) suggestions.

One philosophy I see is not wanting to add the prefix when possible. The other
is to always refer to a name consistently. Complicating this is that several
names are in both namespace llvm and Ice.

After talking with Jim, I've concluded that code should at most, use one
namespace, so that there isn't hidden name clashes. For the moment,
I have fixed cases which allow multiple namespaces. Rather I have put this off
to a separate CL.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:59: public:
On 2014/10/10 01:47:27, jvoung wrote:
> already in the public: section?

Removed.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:60: Ice::Converter &getConverter() { return Converter; }
On 2014/10/10 01:47:28, jvoung wrote:
> const ?

Done.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:693: const auto GV = dyn_cast<GlobalVariable>(I);
On 2014/10/10 01:47:28, jvoung wrote:
> cast<> is a checked cast and would help assert(GV); ?

Ok. Changing to cast and removing assert.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:704: assert(llvm::isa<Ice::GlobalVariable>(Addr));
On 2014/10/10 01:47:28, jvoung wrote:
> sometimes llvm:: is used, but sometimes not?

Assuming we should always use on GlobalVariable, since it is in both namespaces.

Note that I have renamed Ice::GlobalVariable to Ice::VariableDeclaration,
avoiding namespace clashes.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:705: auto VarAddr = llvm::cast<Ice::GlobalVariable>(Addr);
On 2014/10/10 01:47:28, jvoung wrote:
> cast<> already asserts isa<>, so the earlier assert for isa<> is redundant.

Removing redundancy.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:761: if (Addr == NULL) {
On 2014/10/10 01:47:28, jvoung wrote:
> nullptr

Replaced all occurrences.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:806: for (llvm::Function &Func : *Mod) {
On 2014/10/10 01:47:28, jvoung wrote:
> could this be const ?

Done.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:824: const auto GV = dyn_cast<llvm::GlobalVariable>(I);
On 2014/10/10 01:47:28, jvoung wrote:
> could just cast<> to assert that GV is really a GlobalVariable (instead of the
> assert right after this?)

Fixed use of cast. Using explicit namespace prefix for GlobalVariable, since it
can appear in both llvm and Ice.

Note: This is no longer an issue since I am now using Ice::VariableDeclaration
instead of Ice::GlobalVariable.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:831: Vbl->setIsInternal(!GV->isExternallyInitialized());
On 2014/10/10 01:47:27, jvoung wrote:
> Was there a commandline flag to check?

Yes.

Will do in a separate CL, since many lit tests will require the addition of the
flag. Adding TODO.

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.h#newc...
src/IceConverter.h:34: ~Converter() {}
On 2014/10/10 01:47:28, jvoung wrote:
> override?

Why. A translator doesn't have any virtual methods. Are you suggesting that any
class that can have derived classes should have a virtual destructor?

https://codereview.chromium.org/641193002/diff/150001/src/IceConverter.h#newc...
src/IceConverter.h:40: llvm::Module *getModule() { return Mod; }
On 2014/10/10 01:47:28, jvoung wrote:
> method is "const"?

Done.

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:404: GlobalVariable *Vbl = new GlobalVariable();
On 2014/10/10 01:47:28, jvoung wrote:
> Most abbreviations of "Variable" are "Var" in the other code.

Done.

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:24: #include "IceClFlags.h"
On 2014/10/10 13:15:00, stichnot wrote:
> Is this new include necessary?

See line 220. On that line it uses Ctx>getFlags().SubzeroTimiingEnabled. Hence,
yes, it is needed (it didn't create a problem because before this CL, all files
that included this include file also included IceClFlags.h).

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.cpp
File src/IceGlobalInits.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.cpp#...
src/IceGlobalInits.cpp:53: }
On 2014/10/10 13:15:00, stichnot wrote:
> // end of anonymous namespace

Done.

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.cpp#...
src/IceGlobalInits.cpp:186: }
On 2014/10/10 13:15:00, stichnot wrote:
> // end of namespace Ice

Done.

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:69: // it's calling conventions, and its linkage.
On 2014/10/10 13:15:00, stichnot wrote:
> its

Done.

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:70: class Function : public GlobalAddress {
On 2014/10/10 13:15:00, stichnot wrote:
> Could we use a different name than "Function" for this?  When I read it, it
> makes me think of a fully defined function, like what Cfg captures.

Changing as follows:

GlobalAddress -> GlobalDeclaration
Function -> FunctionDeclaration
GlobalVariable -> VariableDeclaration

Chose these names to clarify usage as declarations, make names easy to
differentiate, and to not conflict between namespaces.

https://codereview.chromium.org/641193002/diff/150001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:108: /// Models a global address, and its initializers.
On 2014/10/10 01:47:28, jvoung wrote:
> nit: How about "global variable address" otherwise, it looks like the
> "GlobalAddress" class?

See renaming above.

https://codereview.chromium.org/641193002/diff/150001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/641193002/diff/150001/src/IceTargetLowering.h...
src/IceTargetLowering.h:252: virtual void lower(const GlobalVariable &Vbl, bool
DisableTranslation) = 0;
On 2014/10/10 01:47:28, jvoung wrote:
> nit: How about "Var" instead of "Vbl"?

Done.

https://codereview.chromium.org/641193002/diff/150001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/641193002/diff/150001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:266: const FunctionListType &getFunctionList() { return
FunctionList; }
On 2014/10/10 01:47:28, jvoung wrote:
> const ?

Done.

https://codereview.chromium.org/641193002/diff/150001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:427: // Reports that there is no function address for
ID. Returns
On 2014/10/10 13:15:00, stichnot wrote:
> fill-paragraph?

Done.

https://codereview.chromium.org/641193002/diff/150001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/globalrelocs.ll (right):

https://codereview.chromium.org/641193002/diff/150001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/globalrelocs.ll:128: ; CHECK:	.long	ptr + -1
On 2014/10/10 13:15:00, stichnot wrote:
> Maybe the emit code could make this a little prettier? :)

Done. Note that "ptr + INT32_MIN" will not be prettier since we can't represent
INT32_MIN using its absolute value.

[jvoung (off chromium), 2014-10-10 23:01:16]

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:647: /// addresses, for module Mod. Puts corresponding
converted global
Sorry about the churn, but could convert the comments saying "global addressed"
to say "variable declarations"

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:834: // Install function addresses.
addresses -> declarations

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.h#newc...
src/IceConverter.h:30: GlobalDeclarationMapType;
It might be possible to make this typedef private, since the map is only used
internally?

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:397: FunctionDeclaration *Func = new
FunctionDeclaration(
This could potentially use: new (allocate<FunctionDeclaration>())
FunctionDeclaration(...), if you want to use the slab allocator.

W.r.t. future threading, I guess this part will be done before threads are
created and take their stab at translation.

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.cpp
File src/IceGlobalInits.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.cpp#...
src/IceGlobalInits.cpp:60: return (getIsExternal() && !hasInitializer()) ? Name
: Ctx->mangleName(Name);
Is it possible that IsExternal can hasInitializer()?

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:31: /// Base class for global varialbe and function
declarations.
varialbe -> variable

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:211: RelocInitializer(const GlobalDeclaration *Declaration,
RelocOffsetType Offset)
82 col?

https://codereview.chromium.org/641193002/diff/360001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:18: #include <stdint.h>
<cstdint>

This should come from IceDefs already?

https://codereview.chromium.org/641193002/diff/360001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:87: bool DisplayInsts = Ctx->isVerbose();
Insts or Inits? (is this referring to initializers)

https://codereview.chromium.org/641193002/diff/360001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:400: // The list of function declaraion IDs (in the
order found) that
declaration

https://codereview.chromium.org/641193002/diff/360001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2407: void installDeclartionName(Ice::Translator &Trans,
installDeclarationName

https://codereview.chromium.org/641193002/diff/360001/tests_lit/reader_tests/...
File tests_lit/reader_tests/globalinit.pnacl.ll (right):

https://codereview.chromium.org/641193002/diff/360001/tests_lit/reader_tests/...
tests_lit/reader_tests/globalinit.pnacl.ll:1: ; Test of global initializers.
There was also a copy of this in llvm2ice_tests. Do we need both? Do you mind if
I remove one of them?

[Karl, 2014-10-13 17:43:02]

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:647: /// addresses, for module Mod. Puts corresponding
converted global
On 2014/10/10 23:01:16, jvoung wrote:
> Sorry about the churn, but could convert the comments saying "global
addressed"
> to say "variable declarations"

Done.

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:834: // Install function addresses.
On 2014/10/10 23:01:16, jvoung wrote:
> addresses -> declarations

Done.

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceConverter.h#newc...
src/IceConverter.h:30: GlobalDeclarationMapType;
On 2014/10/10 23:01:16, jvoung wrote:
> It might be possible to make this typedef private, since the map is only used
> internally?

Done.

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:397: FunctionDeclaration *Func = new
FunctionDeclaration(
On 2014/10/10 23:01:16, jvoung wrote:
> This could potentially use: new (allocate<FunctionDeclaration>())
> FunctionDeclaration(...), if you want to use the slab allocator.
> 
> W.r.t. future threading, I guess this part will be done before threads are
> created and take their stab at translation.

The goal is to eventually move to a memory pool and use the new form you
suggest. However, since GlobalVariableDeclarations's still need to be
destructed, I don't want to do such changes yet.

As far as threading, yes this should be done before the threads are created
(since they define the signature of the function).

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.cpp
File src/IceGlobalInits.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.cpp#...
src/IceGlobalInits.cpp:60: return (getIsExternal() && !hasInitializer()) ? Name
: Ctx->mangleName(Name);
On 2014/10/10 23:01:16, jvoung wrote:
> Is it possible that IsExternal can hasInitializer()?

It can if you are assuming the block definition as an initializer.

I originally factored out getIsExternal and hasInitializer as virtuals, so that
we could use it to mangleName in one context. I realized later that there were
several spots that we want the same effect, and factored out this function.

Changing so that hasInitializer is not virtual, and making mangleName virtual.
This should clean this up.

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:31: /// Base class for global varialbe and function
declarations.
On 2014/10/10 23:01:16, jvoung wrote:
> varialbe -> variable

Done.

https://codereview.chromium.org/641193002/diff/360001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:211: RelocInitializer(const GlobalDeclaration *Declaration,
RelocOffsetType Offset)
On 2014/10/10 23:01:16, jvoung wrote:
> 82 col?

Done.

https://codereview.chromium.org/641193002/diff/360001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:18: #include <stdint.h>
On 2014/10/10 23:01:16, jvoung wrote:
> <cstdint>
> 
> This should come from IceDefs already?

Removed.

https://codereview.chromium.org/641193002/diff/360001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/IceTranslator.cpp#n...
src/IceTranslator.cpp:87: bool DisplayInsts = Ctx->isVerbose();
On 2014/10/10 23:01:16, jvoung wrote:
> Insts or Inits? (is this referring to initializers)

Renamed to DumpGlobalVariables.

https://codereview.chromium.org/641193002/diff/360001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/641193002/diff/360001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:400: // The list of function declaraion IDs (in the
order found) that
On 2014/10/10 23:01:16, jvoung wrote:
> declaration

Done.

https://codereview.chromium.org/641193002/diff/360001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2407: void installDeclartionName(Ice::Translator &Trans,
On 2014/10/10 23:01:16, jvoung wrote:
> installDeclarationName

Done.

https://codereview.chromium.org/641193002/diff/360001/tests_lit/reader_tests/...
File tests_lit/reader_tests/globalinit.pnacl.ll (right):

https://codereview.chromium.org/641193002/diff/360001/tests_lit/reader_tests/...
tests_lit/reader_tests/globalinit.pnacl.ll:1: ; Test of global initializers.
On 2014/10/10 23:01:16, jvoung wrote:
> There was also a copy of this in llvm2ice_tests. Do we need both? Do you mind
if
> I remove one of them?

I agree that these two tests are doing the same thing, but they shouldn't. This
should only be testing parsing. The other should only be testing translation.
Fixing this file to only test parsing.

[Jim Stichnoth, 2014-10-13 19:14:23]

https://codereview.chromium.org/641193002/diff/490001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/641193002/diff/490001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:726: bool HasOffset,
Ice::VariableDeclaration::RelocOffsetType Offset) {
The NOASSERT=1 builds are warning that HasOffset is an unused parameter.  Could
you add "(void)HasOffset;" somewhere?

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:15: #include "llvm/ADT/STLExtras.h"
I think it's worth factoring this include into IceDefs.h.

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:23: #include "llvm/IR/CallingConv.h"
put these includes first

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:52: // virtual bool hasInitializer() const = 0;
Not sure why this method is commented out here and in FunctionDeclaration, but
not in VariableDeclaration.

[Karl, 2014-10-13 20:48:04]

https://codereview.chromium.org/641193002/diff/490001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/641193002/diff/490001/src/IceConverter.cpp#ne...
src/IceConverter.cpp:726: bool HasOffset,
Ice::VariableDeclaration::RelocOffsetType Offset) {
On 2014/10/13 19:14:22, stichnot wrote:
> The NOASSERT=1 builds are warning that HasOffset is an unused parameter. 
Could
> you add "(void)HasOffset;" somewhere?

Done.

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:15: #include "llvm/ADT/STLExtras.h"
On 2014/10/13 19:14:22, stichnot wrote:
> I think it's worth factoring this include into IceDefs.h.

Done.

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:23: #include "llvm/IR/CallingConv.h"
On 2014/10/13 19:14:22, stichnot wrote:
> put these includes first

Done.

https://codereview.chromium.org/641193002/diff/490001/src/IceGlobalInits.h#ne...
src/IceGlobalInits.h:52: // virtual bool hasInitializer() const = 0;
On 2014/10/13 19:14:22, stichnot wrote:
> Not sure why this method is commented out here and in FunctionDeclaration, but
> not in VariableDeclaration.

Initialization (as pointed out by Jan) is a VariableDeclaration only concept.
Hence, I have removed it from the base class. Unfortunately, I forgot to remove
the commented out code. Removing.

[jvoung (off chromium), 2014-10-13 21:25:43]

lgtm

https://codereview.chromium.org/641193002/diff/360001/tests_lit/reader_tests/...
File tests_lit/reader_tests/globalinit.pnacl.ll (right):

https://codereview.chromium.org/641193002/diff/360001/tests_lit/reader_tests/...
tests_lit/reader_tests/globalinit.pnacl.ll:1: ; Test of global initializers.
On 2014/10/13 17:43:02, Karl wrote:
> On 2014/10/10 23:01:16, jvoung wrote:
> > There was also a copy of this in llvm2ice_tests. Do we need both? Do you
mind
> if
> > I remove one of them?
> 
> I agree that these two tests are doing the same thing, but they shouldn't.
This
> should only be testing parsing. The other should only be testing translation.
> Fixing this file to only test parsing.

Thanks!

I think that the " .att_syntax" etc, parts below which are x86-specific can be
removed too then.

Undecided about what .section .data parts, but I guess we can revisit that when
doing direct-to-object.

[Jim Stichnoth, 2014-10-13 21:29:06]

also LGTM

[Karl, 2014-10-13 22:01:17]

Committed patchset #7 (id:920001) manually as 9d98d79 (presubmit successful).

[Karl, 2014-10-13 22:01:32]

https://codereview.chromium.org/641193002/diff/360001/tests_lit/reader_tests/...
File tests_lit/reader_tests/globalinit.pnacl.ll (right):

https://codereview.chromium.org/641193002/diff/360001/tests_lit/reader_tests/...
tests_lit/reader_tests/globalinit.pnacl.ll:1: ; Test of global initializers.
On 2014/10/13 21:25:43, jvoung wrote:
> On 2014/10/13 17:43:02, Karl wrote:
> > On 2014/10/10 23:01:16, jvoung wrote:
> > > There was also a copy of this in llvm2ice_tests. Do we need both? Do you
> mind
> > if
> > > I remove one of them?
> > 
> > I agree that these two tests are doing the same thing, but they shouldn't.
> This
> > should only be testing parsing. The other should only be testing
translation.
> > Fixing this file to only test parsing.
> 
> Thanks!
> 
> I think that the " .att_syntax" etc, parts below which are x86-specific can be
> removed too then.
> 
> Undecided about what .section .data parts, but I guess we can revisit that
when
> doing direct-to-object.

Removed the translated code.