Subzero: Simplify the constant pools.

Subzero: Simplify the constant pools.

Internally, create a separate constant pool for each integer type, instead of a single i64 pool that uses the Ice::Type value as part of the key.  This means each constant pool key can be a simple primitive value, rather than a tuple.

Represent the pools using std::unordered_map instead of std::map since we're using C++11 now.

Use signed integers instead of unsigned integers for the integer constant pools, to benefit from sign extension and to be more consistent.

Remove the SuppressMangling field from hash and comparison functions on RelocatableTuple, since we'll never have two symbols with the same name but different values of SuppressMangling.

BUG= none
R=jvoung@chromium.org

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

[Jim Stichnoth, 2014-11-19 13:44:10]

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

[Jim Stichnoth, 2014-11-19 16:35:08]

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

[Jim Stichnoth, 2014-11-19 18:18:39]

https://codereview.chromium.org/737513008/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/737513008/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:31: return std::hash<Ice::IceString>()(Key.Name) +
Not sure this is the ideal hash function.  However, Offset will pretty much
always be 0, occasionally 4.

https://codereview.chromium.org/737513008/diff/40001/src/assembler_ia32.cpp
File src/assembler_ia32.cpp (right):

https://codereview.chromium.org/737513008/diff/40001/src/assembler_ia32.cpp#n...
src/assembler_ia32.cpp:2166: assert(imm.is_uint16());
I'm not sure if this should be changed to is_int16().

(Not that it really matters since we will probably never use this version of the
ret instruction, and almost certainly never use a value more than 32768.)

[Karl, 2014-11-19 18:45:28]

https://codereview.chromium.org/737513008/diff/40001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/737513008/diff/40001/src/IceConverter.cpp#new...
src/IceConverter.cpp:123: switch (Ty) {
Why not move this switch into IceGlobalcontext.{h,cpp}.

https://codereview.chromium.org/737513008/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (left):

https://codereview.chromium.org/737513008/diff/40001/src/IceGlobalContext.h#o...
src/IceGlobalContext.h:116: Constant *getConstantInt32(Type Ty, uint32_t
ConstantInt32);
Why not have a version of this defined here (with the corresponding switch),
rather than force callers to do this.

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

https://codereview.chromium.org/737513008/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2503: switch (NextConstantType) {
Why not provide a helper function (in IceGlobalContext.h) that does this.

[jvoung (off chromium), 2014-11-19 20:54:56]

https://codereview.chromium.org/737513008/diff/40001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/737513008/diff/40001/src/IceOperand.cpp#newco...
src/IceOperand.cpp:24: return A.Offset == B.Offset && A.Name == B.Name;
This used to check SuppressMangling as well. It does seem like there shouldn't
be a conflict w/ not checking that, so should be okay, but just wanted to check.
Same with the hash function.

https://codereview.chromium.org/737513008/diff/40001/src/assembler_ia32.cpp
File src/assembler_ia32.cpp (right):

https://codereview.chromium.org/737513008/diff/40001/src/assembler_ia32.cpp#n...
src/assembler_ia32.cpp:2166: assert(imm.is_uint16());
On 2014/11/19 18:18:39, stichnot wrote:
> I'm not sure if this should be changed to is_int16().
> 
> (Not that it really matters since we will probably never use this version of
the
> ret instruction, and almost certainly never use a value more than 32768.)

Hmm yeah we're probably not going to use it. I think the only time I've seen it
was for returning a struct by value, and that just needs to pop a pointer w/ a
positive 4. I guess it can be left alone to keep the diff similar to the Dart
version?

[Jim Stichnoth, 2014-11-20 00:08:57]

https://codereview.chromium.org/737513008/diff/40001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/737513008/diff/40001/src/IceConverter.cpp#new...
src/IceConverter.cpp:123: switch (Ty) {
On 2014/11/19 18:45:28, Karl wrote:
> Why not move this switch into IceGlobalcontext.{h,cpp}.

Done.

https://codereview.chromium.org/737513008/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (left):

https://codereview.chromium.org/737513008/diff/40001/src/IceGlobalContext.h#o...
src/IceGlobalContext.h:116: Constant *getConstantInt32(Type Ty, uint32_t
ConstantInt32);
On 2014/11/19 18:45:28, Karl wrote:
> Why not have a version of this defined here (with the corresponding switch),
> rather than force callers to do this.

Done, see the new getConstantInt() method.

https://codereview.chromium.org/737513008/diff/40001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/737513008/diff/40001/src/IceOperand.cpp#newco...
src/IceOperand.cpp:24: return A.Offset == B.Offset && A.Name == B.Name;
On 2014/11/19 20:54:55, jvoung wrote:
> This used to check SuppressMangling as well. It does seem like there shouldn't
> be a conflict w/ not checking that, so should be okay, but just wanted to
check.
> Same with the hash function.

You're right, I'll update the commit message.

We will never have two (relocatable) symbols with the same name where one
version needs mangling and the other version does not.  The ones that need
mangling are globals with definitions/initializations in this pexe.  The ones
that don't need mangling are helper functions, and external globals without
definitions/initializations.  So there's no benefit to including the
SuppressMangling field in the tuple comparison and hash functions.  The fact
that we used it before was either always unnecessary, or covering over some
other misunderstanding or incorrect implementation.

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

https://codereview.chromium.org/737513008/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2503: switch (NextConstantType) {
On 2014/11/19 18:45:28, Karl wrote:
> Why not provide a helper function (in IceGlobalContext.h) that does this.

Done.

https://codereview.chromium.org/737513008/diff/40001/src/assembler_ia32.cpp
File src/assembler_ia32.cpp (right):

https://codereview.chromium.org/737513008/diff/40001/src/assembler_ia32.cpp#n...
src/assembler_ia32.cpp:2166: assert(imm.is_uint16());
On 2014/11/19 20:54:55, jvoung wrote:
> On 2014/11/19 18:18:39, stichnot wrote:
> > I'm not sure if this should be changed to is_int16().
> > 
> > (Not that it really matters since we will probably never use this version of
> the
> > ret instruction, and almost certainly never use a value more than 32768.)
> 
> Hmm yeah we're probably not going to use it. I think the only time I've seen
it
> was for returning a struct by value, and that just needs to pop a pointer w/ a
> positive 4. I guess it can be left alone to keep the diff similar to the Dart
> version?

Oh, it didn't occur to me that my int8/uint8 mods in this file were against the
original Dart source.  Is that OK, or should I try to attack it from a different
angle?

[jvoung (off chromium), 2014-11-20 00:57:12]

https://codereview.chromium.org/737513008/diff/40001/src/assembler_ia32.cpp
File src/assembler_ia32.cpp (right):

https://codereview.chromium.org/737513008/diff/40001/src/assembler_ia32.cpp#n...
src/assembler_ia32.cpp:2166: assert(imm.is_uint16());
On 2014/11/20 00:08:57, stichnot wrote:
> On 2014/11/19 20:54:55, jvoung wrote:
> > On 2014/11/19 18:18:39, stichnot wrote:
> > > I'm not sure if this should be changed to is_int16().
> > > 
> > > (Not that it really matters since we will probably never use this version
of
> > the
> > > ret instruction, and almost certainly never use a value more than 32768.)
> > 
> > Hmm yeah we're probably not going to use it. I think the only time I've seen
> it
> > was for returning a struct by value, and that just needs to pop a pointer w/
a
> > positive 4. I guess it can be left alone to keep the diff similar to the
Dart
> > version?
> 
> Oh, it didn't occur to me that my int8/uint8 mods in this file were against
the
> original Dart source.  Is that OK, or should I try to attack it from a
different
> angle?


Well, this ret() is from the original Dart source. Some of the others are and
some are not. See the comment about the test() method though -- maybe that will
be a reason not to change it? Does the Ice Type const pool separation in the
original Key matter for integers or only for floats vs doubles?

https://codereview.chromium.org/737513008/diff/60001/src/assembler_ia32.cpp
File src/assembler_ia32.cpp (right):

https://codereview.chromium.org/737513008/diff/60001/src/assembler_ia32.cpp#n...
src/assembler_ia32.cpp:1524: if (immediate.is_int8() && reg < 4) {
Hmm after a second look I'm a little wary of this one, since the immediate is
supposed to represent a bit mask, and it's trying to optimize for when the upper
bits are actually 0.

[Karl, 2014-11-20 17:43:39]

Other than nit below, looks good to me. However, I will let Jan comment on the
assembly changes.

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

https://codereview.chromium.org/737513008/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2502: Ice::Constant *C = nullptr;
Why not:

if (auto C = getContext->getConstantInt(...)) {
  FuncParser->setNextConstantID(C);
  return;
}

[Jim Stichnoth, 2014-11-20 19:04:58]

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

https://codereview.chromium.org/737513008/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2502: Ice::Constant *C = nullptr;
On 2014/11/20 17:43:39, Karl wrote:
> Why not:
> 
> if (auto C = getContext->getConstantInt(...)) {
>   FuncParser->setNextConstantID(C);
>   return;
> }

Good point, done.

https://codereview.chromium.org/737513008/diff/60001/src/assembler_ia32.cpp
File src/assembler_ia32.cpp (right):

https://codereview.chromium.org/737513008/diff/60001/src/assembler_ia32.cpp#n...
src/assembler_ia32.cpp:1524: if (immediate.is_int8() && reg < 4) {
On 2014/11/20 00:57:12, jvoung wrote:
> Hmm after a second look I'm a little wary of this one, since the immediate is
> supposed to represent a bit mask, and it's trying to optimize for when the
upper
> bits are actually 0.

OK, I think I got to the bottom of this.

The root cause is that the target lowering is creating 8-bit immediate constants
to represent masks and other unsigned 8-bit immediates, and when the value is
between 128:255, it ends up sign-extended in the internal representation and the
is_uint8() test fails.  The solution (or at least one easy solution) is for the
lowering to create these constants as a wider type like 32-bit so that sign
extension doesn't corrupt the bit pattern.

With that solution, I reverted all the int8 changes in the file, as well as all
the changes in one of the lit tests.

[jvoung (off chromium), 2014-11-20 19:09:59]

LGTM

[Jim Stichnoth, 2014-11-20 19:24:46]

Committed patchset #5 (id:80001) manually as
d2cb4361c732dcddc98659415f37be45982e20c3 (presubmit successful).