Commit 5bfe2157 by Jim Stichnoth

Subzero: Fix floating-point constant pooling.

This fixes a regression likely introduced in d2cb4361 . 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 Review URL: https://codereview.chromium.org/1019233002
parent 8c980d0d
......@@ -40,6 +40,30 @@ template <> struct hash<Ice::RelocatableTuple> {
namespace Ice {
namespace {
// Define the key comparison function for the constant pool's
// unordered_map, but only for key types of interest: integer types,
// floating point types, and the special RelocatableTuple.
template <typename KeyType, class Enable = void> struct KeyCompare {};
template <typename KeyType>
struct KeyCompare<KeyType,
typename std::enable_if<
std::is_integral<KeyType>::value ||
std::is_same<KeyType, RelocatableTuple>::value>::type> {
bool operator()(const KeyType &Value1, const KeyType &Value2) const {
return Value1 == Value2;
}
};
template <typename KeyType>
struct KeyCompare<KeyType, typename std::enable_if<
std::is_floating_point<KeyType>::value>::type> {
bool operator()(const KeyType &Value1, const KeyType &Value2) const {
return !memcmp(&Value1, &Value2, sizeof(KeyType));
}
};
// TypePool maps constants of type KeyType (e.g. float) to pointers to
// type ValueType (e.g. ConstantFloat).
template <Type Ty, typename KeyType, typename ValueType> class TypePool {
......@@ -65,7 +89,15 @@ public:
}
private:
typedef std::unordered_map<KeyType, ValueType *> ContainerType;
// Use the default hash function, and a custom key comparison
// function. The key comparison function for floating point
// variables can't use the default == based implementation because
// of special C++ semantics regarding +0.0, -0.0, and NaN
// comparison. However, it's OK to use the default hash for
// floating point values because KeyCompare is the final source of
// truth - in the worst case a "false" collision must be resolved.
typedef std::unordered_map<KeyType, ValueType *, std::hash<KeyType>,
KeyCompare<KeyType>> ContainerType;
ContainerType Pool;
uint32_t NextPoolID;
};
......@@ -89,6 +121,8 @@ private:
std::vector<ConstantUndef *> Pool;
};
} // end of anonymous namespace
// The global constant pool bundles individual pools of each type of
// interest.
class ConstantPool {
......
; This file is copied/adapted from llvm/test/NaCl/PNaClABI/abi-atomics.ll .
; TODO(stichnot): Find a way to share the file to avoid divergence.
; REQUIRES: allow_dump
; RUN: %p2i -i %s --args --verbose none --exit-success -threads=0 2>&1 \
; RUN: | FileCheck %s
......
; This tests that different floating point constants (such as 0.0 and -0.0)
; remain distinct even when they sort of look equal, and also that different
; instances of the same floating point constant (such as NaN and NaN) get the
; same constant pool entry even when "a==a" would suggest they are different.
; REQUIRES: allow_dump
define void @consume_float(float %f) {
ret void
}
define void @consume_double(double %d) {
ret void
}
define void @test_zeros() {
entry:
call void @consume_float(float 0.0)
call void @consume_float(float -0.0)
call void @consume_double(double 0.0)
call void @consume_double(double -0.0)
ret void
}
; Parse the function, dump the bitcode back out, and stop without translating.
; This tests that +0.0 and -0.0 aren't accidentally merged into a single
; zero-valued constant pool entry.
;
; RUN: %p2i -i %s --insts | FileCheck --check-prefix=ZERO %s
; ZERO: test_zeros
; ZERO-NEXT: entry:
; ZERO-NEXT: call void @consume_float(float 0.0
; ZERO-NEXT: call void @consume_float(float -0.0
; ZERO-NEXT: call void @consume_double(double 0.0
; ZERO-NEXT: call void @consume_double(double -0.0
define void @test_nans() {
entry:
call void @consume_float(float 0x7FF8000000000000)
call void @consume_float(float 0x7FF8000000000000)
call void @consume_float(float 0xFFF8000000000000)
call void @consume_float(float 0xFFF8000000000000)
call void @consume_double(double 0x7FF8000000000000)
call void @consume_double(double 0x7FF8000000000000)
call void @consume_double(double 0xFFF8000000000000)
call void @consume_double(double 0xFFF8000000000000)
ret void
}
; The following tests check the emitted constant pool entries and make sure
; there is at most one entry for each NaN value. We have to run a separate test
; for each NaN because the constant pool entries may be emitted in any order.
;
; RUN: %p2i -i %s --filetype=asm --llvm-source \
; RUN: | FileCheck --check-prefix=NANS1 %s
; NANS1: float nan
; NANS1-NOT: float nan
;
; RUN: %p2i -i %s --filetype=asm --llvm-source \
; RUN: | FileCheck --check-prefix=NANS2 %s
; NANS2: float -nan
; NANS2-NOT: float -nan
;
; RUN: %p2i -i %s --filetype=asm --llvm-source \
; RUN: | FileCheck --check-prefix=NANS3 %s
; NANS3: double nan
; NANS3-NOT: double nan
;
; RUN: %p2i -i %s --filetype=asm --llvm-source \
; RUN: | FileCheck --check-prefix=NANS4 %s
; NANS4: double -nan
; NANS4-NOT: double -nan
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment