Introduce model of global initializers in Subzero.

Introduce model of global initializers in Subzero.

Modifies both LLVM to ICE converter, and Subzero's bitcode reader,
to build Subzero's global initializers. Modifies target lowering
routines for global initializers to use this new model.

Also modifies both to now handle relocations in global variable
initializers.

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

[Karl, 2014-10-02 20:05:55]

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

[Jim Stichnoth, 2014-10-03 01:28:09]

First round of comments.

BTW, I was able to hand-build and run a couple of spec2k components, using only
Subzero and no llc!

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

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:37: using namespace llvm;
Is this needed?  There are a lot of explicit uses of llvm:: below.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:652: std::vector<Ice::GlobalAddress *> &GlobalAddresses);
Can you make a typedef for std::vector<Ice::GlobalAddress *> ?
Maybe std::map<const llvm::GlobalVariable *, Ice::GlobalAddress *> as well.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:655: // Map from global variables to thier corresponding
global address.
their

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:674: Ice::GlobalAddress *Addr = GlobalVarAddressMap[GV];
Maybe better form to do something like this?

  if (GlobalVarAddressMap.find(GV) == GlobalVarAddressMap.end())
    GlobalVarAddressMap[GV] = new Ice::GlobalAddress();
  return GlobalVarAddressMap[GV];

The point being to only use the [] operator on the RHS if you know it has a
valid entry.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:686: const llvm::ConstantInt *CI =
llvm::dyn_cast<llvm::ConstantInt>(C);
if (auto CI = llvm::dyn_cast<const llvm::ConstantInt>(C))
  if (CI->getType()->isIntegerTy(32))
    return CI->getSExtValue();

And BTW, it seems like cast<> and dyn_cast<> are good opportunities for auto.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:707: assert(GV);
If you're going to assert(GV), might as well use llvm::cast<>() above.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:725: for (llvm::ConstantStruct::const_op_iterator i =
CompoundInit->op_begin(),
Use I and E instead of i and e for consistency?

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:10: // This file declares the representation of global
addresses
Reformat?  e.g. "and" ought to fit on this line.

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4443: void TargetGlobalInitX8632::lower(const
GlobalAddress &Global,
Could you also implement a -fdata-sections option?

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4529: Str << "\t.word\t" << Reloc->getName();
.long

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:154: C = nullptr;
Either use NULL, or use nullptr below?

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2167: // Generate names for unnamed global addresses,
and lowers global
either Generates, or lower
for agreement

https://codereview.chromium.org/624663002/diff/20001/tests_lit/reader_tests/g...
File tests_lit/reader_tests/globalrelocs.ll (right):

https://codereview.chromium.org/624663002/diff/20001/tests_lit/reader_tests/g...
tests_lit/reader_tests/globalrelocs.ll:1: ; Tests if we handle global variables
with relocation initializers.
I'd also like to see a basic cross test for global initializers.  The cross test
itself would just be a set of simple functions that return a pointer to some
global data and its size.  The driver would invoke both versions of each and
memcmp() the results.

[jvoung (off chromium), 2014-10-03 16:15:30]

cool!

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

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:58: Ice::TypeConverter TypeConverter;
Could disallow copy / assign.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:660: void AddGlobalInitializer(Ice::GlobalAddress &Global,
method name consistency: "addGlobalInitializer" instead?

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:662: AddGlobalInitializer(Global, Initializer, false, 0);
nit: For some of the other code in Subzero, there is a style of documenting
constants w/ const local vars:

const bool HasOffset = false;
const ... Offset = 0;
foo(..., ..., HasOffset, Offset);

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:704: continue;
For the PNaCl ABI, should we be stricter? I thought "Every global variable must
have an initializer. Each initializer must be either a SimpleElement or a
CompoundElement, defined as follows..."

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:730: }
What are the possible "else" here, and should it be an error? Similar for the
ConstantExpr checks below.

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.cpp
File src/IceGlobalInits.cpp (right):

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.cpp#n...
src/IceGlobalInits.cpp:18: #include "llvm/ADT/STLExtras.h"
nit: LLVM headers before Ice headers?

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:64: Contents[i] = static_cast<uint8_t>(V);
Would it be cleaner to just:
Contents.push_back(...);

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:83: DataVecType Contents;
Disallow copy/assign for these initializer types?

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:89: ZeroInitializer(SizeT Size)
explicit

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:176: /// Addes Initializer to the list of initializers.
Takes ownership of
Adds

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:178: void AddInitializer(Initializer *Initializer) {
naming style: addInitializer(...) ?

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4446: // TODO: Consider moving the dump output
into the driver to be
Is this TODO TODONE?

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.h (left):

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.h:491: bool DisableTranslation) override;
keep the override?

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:584: // Holds the current global address whose
initializer is being
does "defined" fit on the previous line?

https://codereview.chromium.org/624663002/diff/20001/tests_lit/reader_tests/g...
File tests_lit/reader_tests/globalrelocs.ll (right):

https://codereview.chromium.org/624663002/diff/20001/tests_lit/reader_tests/g...
tests_lit/reader_tests/globalrelocs.ll:53: 
Maybe test a constant w/ a compound initializer also?

[Karl, 2014-10-04 16:28:34]

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

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:37: using namespace llvm;
On 2014/10/03 01:28:08, stichnot wrote:
> Is this needed?  There are a lot of explicit uses of llvm:: below.

We are very inconsistent. Removing llvm:: to make consistent.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:58: Ice::TypeConverter TypeConverter;
On 2014/10/03 16:15:29, jvoung wrote:
> Could disallow copy / assign.

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:652: std::vector<Ice::GlobalAddress *> &GlobalAddresses);
On 2014/10/03 01:28:08, stichnot wrote:
> Can you make a typedef for std::vector<Ice::GlobalAddress *> ?
> Maybe std::map<const llvm::GlobalVariable *, Ice::GlobalAddress *> as well.

Added typedef Translator::GlobalAddressList.

Added typedef GlobalVarAddressMapType for GlobalVarAddressMap.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:655: // Map from global variables to thier corresponding
global address.
On 2014/10/03 01:28:08, stichnot wrote:
> their

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:660: void AddGlobalInitializer(Ice::GlobalAddress &Global,
On 2014/10/03 16:15:29, jvoung wrote:
> method name consistency: "addGlobalInitializer" instead?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:662: AddGlobalInitializer(Global, Initializer, false, 0);
On 2014/10/03 16:15:29, jvoung wrote:
> nit: For some of the other code in Subzero, there is a style of documenting
> constants w/ const local vars:
> 
> const bool HasOffset = false;
> const ... Offset = 0;
> foo(..., ..., HasOffset, Offset);
> 

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:674: Ice::GlobalAddress *Addr = GlobalVarAddressMap[GV];
On 2014/10/03 01:28:08, stichnot wrote:
> Maybe better form to do something like this?
> 
>   if (GlobalVarAddressMap.find(GV) == GlobalVarAddressMap.end())
>     GlobalVarAddressMap[GV] = new Ice::GlobalAddress();
>   return GlobalVarAddressMap[GV];
> 
> The point being to only use the [] operator on the RHS if you know it has a
> valid entry.

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:686: const llvm::ConstantInt *CI =
llvm::dyn_cast<llvm::ConstantInt>(C);
On 2014/10/03 01:28:08, stichnot wrote:
> if (auto CI = llvm::dyn_cast<const llvm::ConstantInt>(C))
>   if (CI->getType()->isIntegerTy(32))
>     return CI->getSExtValue();
> 
> And BTW, it seems like cast<> and dyn_cast<> are good opportunities for auto.

Fixed all occurrences in file.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:704: continue;
On 2014/10/03 16:15:29, jvoung wrote:
> For the PNaCl ABI, should we be stricter? I thought "Every global variable
must
> have an initializer. Each initializer must be either a SimpleElement or a
> CompoundElement, defined as follows..."

I agree. I was being lazy because there is a test for an external global. Fixing
test instead.

Note: For the cross test tool, we need to allow uninitialized globals. Fixing by
adding command line flag for cross tests.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:707: assert(GV);
On 2014/10/03 01:28:08, stichnot wrote:
> If you're going to assert(GV), might as well use llvm::cast<>() above.

Not sure what you are asking. Are you asking to put the cast in the assert? I
moved it out because the cased value is used below as well (and did not want to
repeat usage).

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:725: for (llvm::ConstantStruct::const_op_iterator i =
CompoundInit->op_begin(),
On 2014/10/03 01:28:08, stichnot wrote:
> Use I and E instead of i and e for consistency?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceConverter.cpp#new...
src/IceConverter.cpp:730: }
On 2014/10/03 16:15:29, jvoung wrote:
> What are the possible "else" here, and should it be an error? Similar for the
> ConstantExpr checks below.

The format in PNaCl ABI is that the struct is part of the ConstantExpr only if
it has multiple simple entries. If it has a single entry, it is just a simple
entry.

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.cpp
File src/IceGlobalInits.cpp (right):

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.cpp#n...
src/IceGlobalInits.cpp:18: #include "llvm/ADT/STLExtras.h"
On 2014/10/03 16:15:29, jvoung wrote:
> nit: LLVM headers before Ice headers?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:10: // This file declares the representation of global
addresses
On 2014/10/03 01:28:08, stichnot wrote:
> Reformat?  e.g. "and" ought to fit on this line.

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:64: Contents[i] = static_cast<uint8_t>(V);
On 2014/10/03 16:15:29, jvoung wrote:
> Would it be cleaner to just:
> Contents.push_back(...);

The reason I factored it this way was to optimize allocation. By specifying the
size up front, memory is allocated only once.

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:83: DataVecType Contents;
On 2014/10/03 16:15:30, jvoung wrote:
> Disallow copy/assign for these initializer types?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:89: ZeroInitializer(SizeT Size)
On 2014/10/03 16:15:29, jvoung wrote:
> explicit

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:176: /// Addes Initializer to the list of initializers.
Takes ownership of
On 2014/10/03 16:15:30, jvoung wrote:
> Adds

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:178: void AddInitializer(Initializer *Initializer) {
On 2014/10/03 16:15:30, jvoung wrote:
> naming style: addInitializer(...) ?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4443: void TargetGlobalInitX8632::lower(const
GlobalAddress &Global,
On 2014/10/03 01:28:08, stichnot wrote:
> Could you also implement a -fdata-sections option?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4446: // TODO: Consider moving the dump output
into the driver to be
On 2014/10/03 16:15:30, jvoung wrote:
> Is this TODO TODONE?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4529: Str << "\t.word\t" << Reloc->getName();
On 2014/10/03 01:28:08, stichnot wrote:
> .long

Done.

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.h (left):

https://codereview.chromium.org/624663002/diff/20001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.h:491: bool DisableTranslation) override;
On 2014/10/03 16:15:30, jvoung wrote:
> keep the override?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:154: C = nullptr;
On 2014/10/03 01:28:09, stichnot wrote:
> Either use NULL, or use nullptr below?

Changed to use nullptr.

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:584: // Holds the current global address whose
initializer is being
On 2014/10/03 16:15:30, jvoung wrote:
> does "defined" fit on the previous line?

Done.

https://codereview.chromium.org/624663002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2167: // Generate names for unnamed global addresses,
and lowers global
On 2014/10/03 01:28:08, stichnot wrote:
> either Generates, or lower
> for agreement

Done.

https://codereview.chromium.org/624663002/diff/20001/tests_lit/reader_tests/g...
File tests_lit/reader_tests/globalrelocs.ll (right):

https://codereview.chromium.org/624663002/diff/20001/tests_lit/reader_tests/g...
tests_lit/reader_tests/globalrelocs.ll:1: ; Tests if we handle global variables
with relocation initializers.
On 2014/10/03 01:28:09, stichnot wrote:
> I'd also like to see a basic cross test for global initializers.  The cross
test
> itself would just be a set of simple functions that return a pointer to some
> global data and its size.  The driver would invoke both versions of each and
> memcmp() the results.

Done. See crosstest/test_global.cpp

https://codereview.chromium.org/624663002/diff/20001/tests_lit/reader_tests/g...
tests_lit/reader_tests/globalrelocs.ll:53: 
On 2014/10/03 16:15:30, jvoung wrote:
> Maybe test a constant w/ a compound initializer also?

Done.

[Jim Stichnoth, 2014-10-06 18:16:38]

https://codereview.chromium.org/624663002/diff/60001/crosstest/test_global.cpp
File crosstest/test_global.cpp (right):

https://codereview.chromium.org/624663002/diff/60001/crosstest/test_global.cp...
crosstest/test_global.cpp:21: // should have the same relocation value for both
clang and Subzero.
clang --> llc

https://codereview.chromium.org/624663002/diff/60001/crosstest/test_global.cp...
crosstest/test_global.cpp:29: static int ArrayInitPartial[10] = {60, 70, 80, 90,
100};
Is it necessary to change all these to static?  IIRC, I originally made them
non-static so that the various combinations of asm directives would be
exercised.  I realize that the PNaCl bitcode doesn't allow non-static, but this
mechanism is still somewhat important for cross testing, bisection debugging,
etc.

https://codereview.chromium.org/624663002/diff/60001/crosstest/test_global.cp...
crosstest/test_global.cpp:35: // TODO(kschimpf) Add this example once we know
how to not mangle
Could you add a reference to the part in IceTargetLoweringX8632.cpp where the
.long is emitted, to make it clearer what this TODO means?

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:61: Ice::TypeConverter TypeConverter;
Can this be const?

(And while I'm at it, could you change the name TypeConverter::AddLLVMType() to
be addLLVMType()?)

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:70: // If not, all kinds of assertions may fire.
I'd rather read this comment as something a little more mellow like "Otherwise,
the behavior is undefined."

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:638: // If not, all kinds of assertions may fire.
same comment as above

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:776: auto GV = dyn_cast<GlobalValue>(Exp->getOperand(0));
Can any of these autos be const auto?

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.h#newco...
src/IceConverter.h:45: }
// end of namespace Ice

https://codereview.chromium.org/624663002/diff/60001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:68: template <class Data>
I didn't see any uses of this templatized constructor.  Even so, I think maybe
"class Data" should be "class Container" to better reflect the requirement that
it have size(), begin(), and end() methods.

https://codereview.chromium.org/624663002/diff/60001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:142: RelocationKind Kind;
const?

https://codereview.chromium.org/624663002/diff/60001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:174: RelocationAddress Address;
Can these be const?

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:22: #include "IceGlobalInits.h"
I think it's best to try to minimize includes in a .h file (other than including
IceDefs.h and IceTypes.h).  Can you forward-declare class GlobalAddress instead?

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4474: if (Global.getIsConstant()) {
Can you do
  else if (Global.getIsConstant()) {
and skip the following DataSections test?

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4494: if (GlobalAddress::ZeroInitializer
*ZeroInit =
Drive-by suggestion: cast<> and dyn_cast<> with these ginormous type names seem
like a good opportunity for auto.

https://codereview.chromium.org/624663002/diff/60001/src/IceTranslator.h
File src/IceTranslator.h (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceTranslator.h#newc...
src/IceTranslator.h:58: void lowerIceGlobals(const GlobalAddressList
&GlobalAddresses);
maybe lowerGlobals() instead?

https://codereview.chromium.org/624663002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/624663002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:180: C =
getTranslator().getContext()->getConstantSym(getIcePointerType(), 0,
replace 0 with a local symbolic constant

[Karl, 2014-10-06 22:44:05]

https://codereview.chromium.org/624663002/diff/60001/crosstest/test_global.cpp
File crosstest/test_global.cpp (right):

https://codereview.chromium.org/624663002/diff/60001/crosstest/test_global.cp...
crosstest/test_global.cpp:21: // should have the same relocation value for both
clang and Subzero.
On 2014/10/06 18:16:37, stichnot wrote:
> clang --> llc

Done.

https://codereview.chromium.org/624663002/diff/60001/crosstest/test_global.cp...
crosstest/test_global.cpp:29: static int ArrayInitPartial[10] = {60, 70, 80, 90,
100};
On 2014/10/06 18:16:37, stichnot wrote:
> Is it necessary to change all these to static?  IIRC, I originally made them
> non-static so that the various combinations of asm directives would be
> exercised.  I realize that the PNaCl bitcode doesn't allow non-static, but
this
> mechanism is still somewhat important for cross testing, bisection debugging,
> etc.

Removed static keyword.

https://codereview.chromium.org/624663002/diff/60001/crosstest/test_global.cp...
crosstest/test_global.cpp:35: // TODO(kschimpf) Add this example once we know
how to not mangle
On 2014/10/06 18:16:37, stichnot wrote:
> Could you add a reference to the part in IceTargetLoweringX8632.cpp where the
> .long is emitted, to make it clearer what this TODO means?

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:61: Ice::TypeConverter TypeConverter;
On 2014/10/06 18:16:37, stichnot wrote:
> Can this be const?
> 
> (And while I'm at it, could you change the name TypeConverter::AddLLVMType()
to
> be addLLVMType()?)

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:70: // If not, all kinds of assertions may fire.
On 2014/10/06 18:16:37, stichnot wrote:
> I'd rather read this comment as something a little more mellow like
"Otherwise,
> the behavior is undefined."

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:638: // If not, all kinds of assertions may fire.
On 2014/10/06 18:16:38, stichnot wrote:
> same comment as above

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:776: auto GV = dyn_cast<GlobalValue>(Exp->getOperand(0));
On 2014/10/06 18:16:38, stichnot wrote:
> Can any of these autos be const auto?

Converted to const auto.

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.h
File src/IceConverter.h (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceConverter.h#newco...
src/IceConverter.h:45: }
On 2014/10/06 18:16:38, stichnot wrote:
> // end of namespace Ice

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:68: template <class Data>
On 2014/10/06 18:16:38, stichnot wrote:
> I didn't see any uses of this templatized constructor.  Even so, I think maybe
> "class Data" should be "class Container" to better reflect the requirement
that
> it have size(), begin(), and end() methods.

I should be more clear. This constructor gets containers of integer values, and
initializes appropriately. In particular we call this
constructor with both std::vector<uint64_t> and std::vector<uin8_t>.

Clearing this up by choosing template name IntContainer instead of Data.

https://codereview.chromium.org/624663002/diff/60001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:142: RelocationKind Kind;
On 2014/10/06 18:16:38, stichnot wrote:
> const?

Converted to only allow const accessors.

https://codereview.chromium.org/624663002/diff/60001/src/IceGlobalInits.h#new...
src/IceGlobalInits.h:174: RelocationAddress Address;
On 2014/10/06 18:16:38, stichnot wrote:
> Can these be const?

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLowering.h#...
src/IceTargetLowering.h:22: #include "IceGlobalInits.h"
On 2014/10/06 18:16:38, stichnot wrote:
> I think it's best to try to minimize includes in a .h file (other than
including
> IceDefs.h and IceTypes.h).  Can you forward-declare class GlobalAddress
instead?

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4474: if (Global.getIsConstant()) {
On 2014/10/06 18:16:38, stichnot wrote:
> Can you do
>   else if (Global.getIsConstant()) {
> and skip the following DataSections test?

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4494: if (GlobalAddress::ZeroInitializer
*ZeroInit =
On 2014/10/06 18:16:38, stichnot wrote:
> Drive-by suggestion: cast<> and dyn_cast<> with these ginormous type names
seem
> like a good opportunity for auto.

Done.

https://codereview.chromium.org/624663002/diff/60001/src/IceTranslator.h
File src/IceTranslator.h (right):

https://codereview.chromium.org/624663002/diff/60001/src/IceTranslator.h#newc...
src/IceTranslator.h:58: void lowerIceGlobals(const GlobalAddressList
&GlobalAddresses);
On 2014/10/06 18:16:38, stichnot wrote:
> maybe lowerGlobals() instead?

Done.

https://codereview.chromium.org/624663002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/624663002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:180: C =
getTranslator().getContext()->getConstantSym(getIcePointerType(), 0,
On 2014/10/06 18:16:38, stichnot wrote:
> replace 0 with a local symbolic constant

Done.

[Jim Stichnoth, 2014-10-06 22:55:01]

lgtm

https://codereview.chromium.org/624663002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/624663002/diff/80001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:181: C =
getTranslator().getContext()->getConstantSym(getIcePointerType(), Offset,
80-col

[jvoung (off chromium), 2014-10-07 15:35:32]

lgtm

https://codereview.chromium.org/624663002/diff/80001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/624663002/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4496: if (const auto ZeroInit =
llvm::dyn_cast<GlobalAddress::ZeroInitializer>(Init)) {
80 col

https://codereview.chromium.org/624663002/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/global.ll (right):

https://codereview.chromium.org/624663002/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/global.ll:5: ; RUN: %lc2i -i %s --args --verbose inst |
FileCheck %s
Does removing the external, or having this new model happen to fix lc2i? Don't
worry about it for this CL if it doesn't... I'm just checking.

https://codereview.chromium.org/624663002/diff/80001/tests_lit/reader_tests/g...
File tests_lit/reader_tests/globalrelocs.ll (right):

https://codereview.chromium.org/624663002/diff/80001/tests_lit/reader_tests/g...
tests_lit/reader_tests/globalrelocs.ll:55: @const_xaddend_array1 = internal
constant i32 add (i32 ptrtoint ([7 x i8]* @bytes to i32), i32 1)
typo? addend instead of xaddend?

[Karl, 2014-10-07 15:56:22]

https://codereview.chromium.org/624663002/diff/80001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/624663002/diff/80001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:4496: if (const auto ZeroInit =
llvm::dyn_cast<GlobalAddress::ZeroInitializer>(Init)) {
On 2014/10/07 15:35:31, jvoung wrote:
> 80 col

Done.

https://codereview.chromium.org/624663002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/624663002/diff/80001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:181: C =
getTranslator().getContext()->getConstantSym(getIcePointerType(), Offset,
On 2014/10/06 22:55:00, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/624663002/diff/80001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/global.ll (right):

https://codereview.chromium.org/624663002/diff/80001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/global.ll:5: ; RUN: %lc2i -i %s --args --verbose inst |
FileCheck %s
On 2014/10/07 15:35:31, jvoung wrote:
> Does removing the external, or having this new model happen to fix lc2i? Don't
> worry about it for this CL if it doesn't... I'm just checking.

The reason this test was failing to work in p2i was because of the external.
l2ci (i.e. the IceConverter) still allows external symbols. This is intentional.
It makes doing certain types of cross tool tests possible. That is, it allows us
a way to compile the same external reference using llvm2ice and llc, allowing us
to test if they behave the same way.

https://codereview.chromium.org/624663002/diff/80001/tests_lit/reader_tests/g...
File tests_lit/reader_tests/globalrelocs.ll (right):

https://codereview.chromium.org/624663002/diff/80001/tests_lit/reader_tests/g...
tests_lit/reader_tests/globalrelocs.ll:55: @const_xaddend_array1 = internal
constant i32 add (i32 ptrtoint ([7 x i8]* @bytes to i32), i32 1)
On 2014/10/07 15:35:31, jvoung wrote:
> typo? addend instead of xaddend?

Fixed typo.

[Karl, 2014-10-07 17:38:31]

Committed patchset #6 (id:100001) manually as e3f64d0 (presubmit successful).