Convert Subzero's bitcode reader to generate ICE types.

Convert Subzero's bitcode reader to generate ICE types.

Changes Subzero's bitcode reader to build and store ICE types,
instead of using LLVM's types. Note: This code doesn't remove
all uses of LLVM types. They are still used to check types for
instructions and to generate 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=645aa1a9a21d41f523575afad356e76062e9d696

[Karl, 2014-10-04 22:19:50]

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

[Jim Stichnoth, 2014-10-05 15:35:48]

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h#newcode106
src/IceTypes.h:106: /// Models a type signature for a function.
IceIntrinsics uses something similar to this for intrinsic signature validation.
 The main difference is that it uses an explicit array and length instead of a
vector (which may be necessary for static initialization?).  Is it practical to
switch IceIntrinsics to use this class?

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h#newcode107
src/IceTypes.h:107: class FcnSigType {
A little bikeshedding here... Most of the code base (thanks to me, I know) uses
"Func" as an abbreviation for Function, it would be good to keep that
consistent.

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h#newcode115
src/IceTypes.h:115: explicit FcnSigType(Ice::Type ReturnType) :
ReturnType(ReturnType) {}
drop the Ice::

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h#newcode126
src/IceTypes.h:126: void Print(Ostream &Stream) const;
For consistency with the other dump routines:

  void dump(Ostream &Str) const;

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:140: ~TopLevelParser() override {
DeleteContainerPointers(TypeIDValues); }
I think that if possible, the memory allocation strategy here should follow the
arena-based style that Cfg uses for Inst allocation.  This is as long as the
objects being allocated can be made POD, particularly with trivial destructors
which also means no container fields.  (The Inst class deals with this by using
flat arrays and specifying the array size in the constructor.)

If this isn't practical now, at least leave a TODO.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:173: Ice::Type getTypeByID(unsigned ID) {
Can this be const?

I guess not because of setting errors?

I think that these get() methods should either be const, or they should document
why they are not const.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:393: void reportBadSetTypeID(unsigned ID, ExtendedType
*Ty);
const ExtendedType * ?

[jvoung (off chromium), 2014-10-06 14:32:17]

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (left):

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:527: Ty = Context->getLLVMVectorType(Values[0],
can getLLVMVectorType be removed now?

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:173: Ice::Type getTypeByID(unsigned ID) {
I wonder if this should be called "getSimpleTypeByID" to be more specific.

Otherwise it looks close to setTypeID, but that handles both types.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:329: Type *getLLVMIntegerType(unsigned Bits) const {
can this be removed now?

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:380: /// Reports that type ID is undefined.
double vs triple slash consistency

Clarify this is for simple types? Or you could share reportTypeIDAsUndefined()
between the two, but have some parameter that can be used to clarify the error
message.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:386: // Reports that type ID is undefined.
Clarify this is for function signature types?

[Karl, 2014-10-06 21:15:54]

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h#newcode106
src/IceTypes.h:106: /// Models a type signature for a function.
On 2014/10/05 15:35:47, stichnot wrote:
> IceIntrinsics uses something similar to this for intrinsic signature
validation.
>  The main difference is that it uses an explicit array and length instead of a
> vector (which may be necessary for static initialization?).  Is it practical
to
> switch IceIntrinsics to use this class?

After looking at this some more, it appears that we don't want to change the way
intrinsic signatures are represented to maintain POD representation.

Currently, we don't ever compare signatures accross these two models. So I
suggest that we don't worry about this till it becomes an issue. If it does, I
believe that a simple template compare function can be introduced.

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h#newcode107
src/IceTypes.h:107: class FcnSigType {
On 2014/10/05 15:35:47, stichnot wrote:
> A little bikeshedding here... Most of the code base (thanks to me, I know)
uses
> "Func" as an abbreviation for Function, it would be good to keep that
> consistent.

Renamed to FuncSigType.

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h#newcode115
src/IceTypes.h:115: explicit FcnSigType(Ice::Type ReturnType) :
ReturnType(ReturnType) {}
On 2014/10/05 15:35:47, stichnot wrote:
> drop the Ice::

Done.

https://codereview.chromium.org/625243002/diff/40001/src/IceTypes.h#newcode126
src/IceTypes.h:126: void Print(Ostream &Stream) const;
On 2014/10/05 15:35:47, stichnot wrote:
> For consistency with the other dump routines:
> 
>   void dump(Ostream &Str) const;

Done.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (left):

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:527: Ty = Context->getLLVMVectorType(Values[0],
On 2014/10/06 14:32:17, jvoung wrote:
> can getLLVMVectorType be removed now?

Done.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:140: ~TopLevelParser() override {
DeleteContainerPointers(TypeIDValues); }
On 2014/10/05 15:35:47, stichnot wrote:
> I think that if possible, the memory allocation strategy here should follow
the
> arena-based style that Cfg uses for Inst allocation.  This is as long as the
> objects being allocated can be made POD, particularly with trivial destructors
> which also means no container fields.  (The Inst class deals with this by
using
> flat arrays and specifying the array size in the constructor.)
> 
> If this isn't practical now, at least leave a TODO.

Reorganized to no longer allocate extended types. Rather, there is a single
representation (that handles both simple and function signature extended types),
and cast is only used to add accessors. This should solve this problem.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:173: Ice::Type getTypeByID(unsigned ID) {
On 2014/10/05 15:35:47, stichnot wrote:
> Can this be const?
> 
> I guess not because of setting errors?
> 
> I think that these get() methods should either be const, or they should
document
> why they are not const.

Done. Added constness and added qualifier to accessor names.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:329: Type *getLLVMIntegerType(unsigned Bits) const {
On 2014/10/06 14:32:17, jvoung wrote:
> can this be removed now?

Removed getLLVMIntegerType and getLLVMVectorType.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:380: /// Reports that type ID is undefined.
On 2014/10/06 14:32:17, jvoung wrote:
> double vs triple slash consistency
> 
> Clarify this is for simple types? Or you could share reportTypeIDAsUndefined()
> between the two, but have some parameter that can be used to clarify the error
> message.

Simplified all type id look ups to go through one lookup function
getTypeByIDAsKind, and one error report function reportBadTypeIDAs.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:386: // Reports that type ID is undefined.
On 2014/10/06 14:32:17, jvoung wrote:
> Clarify this is for function signature types?

Removed. No longer needed.

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:393: void reportBadSetTypeID(unsigned ID, ExtendedType
*Ty);
On 2014/10/05 15:35:48, stichnot wrote:
> const ExtendedType * ?

Added to reportBadTypeIDAs.

[Jim Stichnoth, 2014-10-06 22:48:52]

https://codereview.chromium.org/625243002/diff/60001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/625243002/diff/60001/src/IceTypes.h#newcode136
src/IceTypes.h:136: void Dump(Ostream &Stream) const;
lowercase dump

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

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:63: explicit ExtendedType() : Kind(Undefined) {}
I don't think you need "explicit" on a zero-arg ctor?

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:68: ExtendedType &operator=(const ExtendedType &Ty) {
This is a shallow copy, so can you just use the default assignment operator
implementation?

Also, disable the default copy ctor?

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:408: // Models an badly indexed undefined extended type.
a badly

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:425: // Note: We turn off constness so that we can
generate an error message.
"... so that we can set ErrorStatus"

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:445: // Note: Reset to make sure we wipe out any changes
(accidentally)
As we discussed in person, try to get rid of const_cast.

[jvoung (off chromium), 2014-10-07 15:54:42]

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/625243002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:329: Type *getLLVMIntegerType(unsigned Bits) const {
On 2014/10/06 21:15:54, Karl wrote:
> On 2014/10/06 14:32:17, jvoung wrote:
> > can this be removed now?
> 
> Removed getLLVMIntegerType and getLLVMVectorType.

Can it be removed from TypeConverter now too?

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

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:102: // Note: For simple types, the return type of the
signature  will
extra space between sig and will

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:234: Ty = const_cast<TopLevelParser
*>(this)->getDummyUndefinedExtendedType();
In this case, how does the result of getDummyUndefinedExtendedType get
associated w/ the ID? (It's not a pointer to an entry in the vector)?

If it's not associated with the ID, then would that affect later calls to
getTypeByIDAsKind?

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:435: // that getUndefinedByTypeID always returns a valid
extended type.
getUndefinedTypeByID?

[Karl, 2014-10-07 20:15:59]

https://codereview.chromium.org/625243002/diff/60001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/625243002/diff/60001/src/IceTypes.h#newcode136
src/IceTypes.h:136: void Dump(Ostream &Stream) const;
On 2014/10/06 22:48:51, stichnot wrote:
> lowercase dump

Done.

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

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:63: explicit ExtendedType() : Kind(Undefined) {}
On 2014/10/06 22:48:52, stichnot wrote:
> I don't think you need "explicit" on a zero-arg ctor?

Done.

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:68: ExtendedType &operator=(const ExtendedType &Ty) {
On 2014/10/06 22:48:52, stichnot wrote:
> This is a shallow copy, so can you just use the default assignment operator
> implementation?
> 
> Also, disable the default copy ctor?

Removed assignment, not needed. Replaced explicit copy constructor with default
constructor.

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:102: // Note: For simple types, the return type of the
signature  will
On 2014/10/07 15:54:42, jvoung wrote:
> extra space between sig and will

Done.

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:234: Ty = const_cast<TopLevelParser
*>(this)->getDummyUndefinedExtendedType();
On 2014/10/07 15:54:41, jvoung wrote:
> In this case, how does the result of getDummyUndefinedExtendedType get
> associated w/ the ID? (It's not a pointer to an entry in the vector)?
> 
> If it's not associated with the ID, then would that affect later calls to
> getTypeByIDAsKind?

This method is called only once for each ID, to get the extended type to define.
If it isn't defined, it is because the count record of the types block was
wrong, and the call to getTypeByIDAsKind will generate the error message. The
call to getDummyUndefinedExtendedType will "reset" the current definition, so
that the previous record will be cleaned. This code is only for error recovery
during the parsing of the types block.

I will try to clarify this in two ways:

1) I will rename this method to clarify it isn't used for general lookup (only
for defining).

2) Rather than reusing, I will go ahead and just extend the types vector, and
use the corresponding vector position. This will guarantee that no corruption
will occur.

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:408: // Models an badly indexed undefined extended type.
On 2014/10/06 22:48:52, stichnot wrote:
> a badly

Removed the need for this field, so this has been removed.

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:425: // Note: We turn off constness so that we can
generate an error message.
On 2014/10/06 22:48:51, stichnot wrote:
> "... so that we can set ErrorStatus"

Done.

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:435: // that getUndefinedByTypeID always returns a valid
extended type.
On 2014/10/07 15:54:41, jvoung wrote:
> getUndefinedTypeByID?

This method has been removed, so nothing to be fixed.

https://codereview.chromium.org/625243002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:445: // Note: Reset to make sure we wipe out any changes
(accidentally)
On 2014/10/06 22:48:52, stichnot wrote:
> As we discussed in person, try to get rid of const_cast.

Done.

[Jim Stichnoth, 2014-10-08 00:36:07]

lgtm

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:437: StrBuf << "Can't find extend type for type id: " <<
ID;
extended?

[jvoung (off chromium), 2014-10-08 02:58:23]

lgtm

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:400: // Cached global variable placeholder type. Used
for all forward
Used for all forward... references ?

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:749: assert(false);
llvm_unreachable?

[Karl, 2014-10-08 16:05:12]

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:400: // Cached global variable placeholder type. Used
for all forward
On 2014/10/08 02:58:22, jvoung wrote:
> Used for all forward... references ?

Fixing comment. It now is only used for error recovery if one asks for a
function signature for a non-function signature extended type.

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:437: StrBuf << "Can't find extend type for type id: " <<
ID;
On 2014/10/08 00:36:07, stichnot wrote:
> extended?

Done.

https://codereview.chromium.org/625243002/diff/100001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:749: assert(false);
On 2014/10/08 02:58:22, jvoung wrote:
> llvm_unreachable?

Done.

[Karl, 2014-10-08 16:06:01]

Committed patchset #7 (id:120001) manually as 645aa1a (tree was closed).