Subzero: Fix floating-point constant pooling.

Subzero: Fix floating-point constant pooling.

This fixes a regression likely introduced in d2cb4361c732dcddc98659415f37be45982e20c3 .

The problem is that by using the default std::unordered_map comparison predicate std::equal_to, we get incorrect behavior when the key is float or double:

1. 0.0 and -0.0 appear equal, so they share a constant pool entry even though the bit patterns are different.  This is a correctness bug.

2. Each instance of NaN gets a separate constant pool entry, because NaN != NaN by C equality rules.  This is a performance bug.  (This problem doesn't show up with the native bitcode reader, because constants are already unique-ified in the PNaCl bitcode file.)

The solution is to use memcmp for floating-point key types.

Also, the abi-atomics.ll test is disabled for the MINIMAL build, to fix an oversight from a previous CL.

BUG= none
R=jfb@chromium.org

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

[Jim Stichnoth, 2015-03-19 08:42:46]

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

[Jim Stichnoth, 2015-03-19 08:46:11]

This and the fabs implementation allow Subzero to pass the LLVM test suite.

[JF, 2015-03-19 16:33:09]

https://codereview.chromium.org/1019233002/diff/1/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1019233002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:76: ContainerType;
template<typename T>
struct Compare {
  typename std::enable_if<std::is_integral<T>::value, bool>::type
  operator()(const KeyType &LHS, const KeyType &RHS) const {
    return LHS == RHS;
  }
  typename std::enable_if<std::is_floating_point<T>::value, bool>::type
  operator()(const KeyType &LHS, const KeyType &RHS) const {
    return !memcmp(&RHS, &LHS, sizeof(KeyType));
  }
};
typedef std::unordered_map<KeyType, ValueType *, std::hash<KeyType>,
Compare<KeyType>>
       ContainerType;

https://codereview.chromium.org/1019233002/diff/1/tests_lit/llvm2ice_tests/ab...
File tests_lit/llvm2ice_tests/abi-atomics.ll (right):

https://codereview.chromium.org/1019233002/diff/1/tests_lit/llvm2ice_tests/ab...
tests_lit/llvm2ice_tests/abi-atomics.ll:4: ; REQUIRES: allow_dump
What's that?

https://codereview.chromium.org/1019233002/diff/1/tests_lit/llvm2ice_tests/fp...
File tests_lit/llvm2ice_tests/fp_const_pool.ll (right):

https://codereview.chromium.org/1019233002/diff/1/tests_lit/llvm2ice_tests/fp...
tests_lit/llvm2ice_tests/fp_const_pool.ll:24: ; RUN: %p2i -i %s --insts |
FileCheck --check-prefix=ZERO %s
Why different RUN lines? It seems like you could just use the one below, not
this one, and encode zero as 0x0000000000000000 and minus zero as
0x8000000000000000.

[Jim Stichnoth, 2015-03-19 18:07:05]

https://codereview.chromium.org/1019233002/diff/1/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1019233002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:76: ContainerType;
On 2015/03/19 16:33:09, JF wrote:
> template<typename T>
> struct Compare {
>   typename std::enable_if<std::is_integral<T>::value, bool>::type
>   operator()(const KeyType &LHS, const KeyType &RHS) const {
>     return LHS == RHS;
>   }
>   typename std::enable_if<std::is_floating_point<T>::value, bool>::type
>   operator()(const KeyType &LHS, const KeyType &RHS) const {
>     return !memcmp(&RHS, &LHS, sizeof(KeyType));
>   }
> };
> typedef std::unordered_map<KeyType, ValueType *, std::hash<KeyType>,
> Compare<KeyType>>
>        ContainerType;

Cool.  I did it a slightly different way.

It has to handle the RelocatableTuple type as well (which already has a valid ==
operator).  Is there any way to combine that with the is_integral<T> test?

https://codereview.chromium.org/1019233002/diff/1/tests_lit/llvm2ice_tests/ab...
File tests_lit/llvm2ice_tests/abi-atomics.ll (right):

https://codereview.chromium.org/1019233002/diff/1/tests_lit/llvm2ice_tests/ab...
tests_lit/llvm2ice_tests/abi-atomics.ll:4: ; REQUIRES: allow_dump
On 2015/03/19 16:33:09, JF wrote:
> What's that?

It means this test doesn't get run in a MINIMAL build because it depends on the
dump stream output which is disabled in a MINIMAL build.  The same thing is done
in a number of other lit tests.

I added it here so that the tests would pass in a MINIMAL build, as I apparently
forgot to test MINIMAL builds in my previous CL.

https://codereview.chromium.org/1019233002/diff/1/tests_lit/llvm2ice_tests/fp...
File tests_lit/llvm2ice_tests/fp_const_pool.ll (right):

https://codereview.chromium.org/1019233002/diff/1/tests_lit/llvm2ice_tests/fp...
tests_lit/llvm2ice_tests/fp_const_pool.ll:24: ; RUN: %p2i -i %s --insts |
FileCheck --check-prefix=ZERO %s
On 2015/03/19 16:33:09, JF wrote:
> Why different RUN lines? It seems like you could just use the one below, not
> this one, and encode zero as 0x0000000000000000 and minus zero as
> 0x8000000000000000.

I tried to clarify this in comments, ptal.  The ZERO test style is preferable
because it very precisely tests whether +/-0.0 have been incorrectly merged. 
For the NANS tests, though, we can't tell from the dump output whether two equal
constant values share a unique pool entry, so we have to rely on the downstream
translation output.

The ZERO test just checks that

[Jim Stichnoth, 2015-03-19 19:57:49]

https://codereview.chromium.org/1019233002/diff/1/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1019233002/diff/1/src/IceGlobalContext.cpp#ne...
src/IceGlobalContext.cpp:76: ContainerType;
On 2015/03/19 18:07:04, stichnot wrote:
> On 2015/03/19 16:33:09, JF wrote:
> > template<typename T>
> > struct Compare {
> >   typename std::enable_if<std::is_integral<T>::value, bool>::type
> >   operator()(const KeyType &LHS, const KeyType &RHS) const {
> >     return LHS == RHS;
> >   }
> >   typename std::enable_if<std::is_floating_point<T>::value, bool>::type
> >   operator()(const KeyType &LHS, const KeyType &RHS) const {
> >     return !memcmp(&RHS, &LHS, sizeof(KeyType));
> >   }
> > };
> > typedef std::unordered_map<KeyType, ValueType *, std::hash<KeyType>,
> > Compare<KeyType>>
> >        ContainerType;
> 
> Cool.  I did it a slightly different way.
> 
> It has to handle the RelocatableTuple type as well (which already has a valid
==
> operator).  Is there any way to combine that with the is_integral<T> test?

Tweaked once more to combine integral types and RelocatableTuple.

[JF, 2015-03-19 20:33:12]

lgtm

https://codereview.chromium.org/1019233002/diff/40001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1019233002/diff/40001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:93: KeyCompare<KeyType>> ContainerType;
Comment on why using std::hash is OK here but the default comparator wasn't.

[Jim Stichnoth, 2015-03-19 20:52:06]

Committed patchset #4 (id:60001) manually as
5bfe21578536c437ef17052038c5d85eaf1bd484 (presubmit successful).

[Jim Stichnoth, 2015-03-20 05:29:49]

https://codereview.chromium.org/1019233002/diff/40001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1019233002/diff/40001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:93: KeyCompare<KeyType>> ContainerType;
On 2015/03/19 20:33:12, JF wrote:
> Comment on why using std::hash is OK here but the default comparator wasn't.

Done.