[webkit-reviews] review granted: [Bug 212106] [Wasm] Reduce the amount of memory used by the Air register coloring allocator : [Attachment 401634] Patch with Build Fixes

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 212106: [Wasm] Reduce the amount of memory used by the Air register
coloring allocator
https://bugs.webkit.org/show_bug.cgi?id=212106

Attachment 401634: Patch with Build Fixes

https://bugs.webkit.org/attachment.cgi?id=401634&action=review




--- Comment #4 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 401634
  --> https://bugs.webkit.org/attachment.cgi?id=401634
Patch with Build Fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=401634&action=review

r=me with comment.

> Source/JavaScriptCore/b3/air/AirAllocateRegistersByGraphColoring.cpp:191
> +class InterferenceEdge16Bit {
> +protected:
> +    using IndexType = unsigned;
> +
> +public:
> +    struct InterferenceEdgeHash {
> +	   static unsigned hash(const InterferenceEdge16Bit& key) { return
key.hash(); }
> +	   static bool equal(const InterferenceEdge16Bit& a, const
InterferenceEdge16Bit& b) { return a == b; }
> +	   static constexpr bool safeToCompareToEmptyOrDeleted = true;
> +    };
> +    typedef SimpleClassHashTraits<InterferenceEdge16Bit>
InterferenceEdgeHashTraits;
> +
> +    typedef HashSet<InterferenceEdge16Bit, InterferenceEdgeHash,
InterferenceEdgeHashTraits> InterferenceSet;
> +
> +    InterferenceEdge16Bit()
> +    {
> +    }
> +
> +    InterferenceEdge16Bit(IndexType a, IndexType b)
> +    {
> +	   ASSERT(a);
> +	   ASSERT(b);
> +	   ASSERT(a < std::numeric_limits<uint16_t>::max());
> +	   ASSERT(b < std::numeric_limits<uint16_t>::max());
> +	   ASSERT_WITH_MESSAGE(a != b, "A Tmp can never interfere with itself.
Doing so would force it to be the superposition of two registers.");
> +
> +	   if (b < a)
> +	       std::swap(a, b);
> +	   m_value = static_cast<uint32_t>(a) << 16 | b;
> +    }
> +
> +    InterferenceEdge16Bit(WTF::HashTableDeletedValueType)
> +	   : m_value(std::numeric_limits<uint32_t>::max())
> +    {
> +    }
> +
> +    IndexType first() const
> +    {
> +	   return m_value >> 16 & 0xffff;
> +    }
> +
> +    IndexType second() const
> +    {
> +	   return m_value & 0xffff;
> +    }
> +
> +    bool operator==(const InterferenceEdge16Bit& other) const
> +    {
> +	   return m_value == other.m_value;
> +    }
> +
> +    bool isHashTableDeletedValue() const
> +    {
> +	   return *this == InterferenceEdge16Bit(WTF::HashTableDeletedValue);
> +    }
> +
> +    unsigned hash() const
> +    {
> +	   return WTF::IntHash<uint32_t>::hash(m_value);
> +    }
> +
> +    void dump(PrintStream& out) const
> +    {
> +	   out.print(first(), "<=>", second());
> +    }
> +
> +private:
> +    uint32_t m_value { 0 };
> +};
> +
> +class InterferenceEdge32Bit {
> +protected:
> +    using IndexType = unsigned;
> +
> +public:
> +    struct InterferenceEdgeHash {
> +	   static unsigned hash(const InterferenceEdge32Bit& key) { return
key.hash(); }
> +	   static bool equal(const InterferenceEdge32Bit& a, const
InterferenceEdge32Bit& b) { return a == b; }
> +	   static constexpr bool safeToCompareToEmptyOrDeleted = true;
> +    };
> +    typedef SimpleClassHashTraits<InterferenceEdge32Bit>
InterferenceEdgeHashTraits;
> +
> +    typedef HashSet<InterferenceEdge32Bit, InterferenceEdgeHash,
InterferenceEdgeHashTraits> InterferenceSet;
> +
> +    InterferenceEdge32Bit()
> +    {
> +    }
> +
> +    InterferenceEdge32Bit(IndexType a, IndexType b)
> +    {
> +	   ASSERT(a);
> +	   ASSERT(b);
> +	   ASSERT_WITH_MESSAGE(a != b, "A Tmp can never interfere with itself.
Doing so would force it to be the superposition of two registers.");
> +
> +	   if (b < a)
> +	       std::swap(a, b);
> +	   m_value = static_cast<uint64_t>(a) << 32 | b;
> +    }
> +
> +    InterferenceEdge32Bit(WTF::HashTableDeletedValueType)
> +	   : m_value(std::numeric_limits<uint64_t>::max())
> +    {
> +    }
> +
> +    IndexType first() const
> +    {
> +	   return m_value >> 32 & 0xffffffff;
> +    }
> +
> +    IndexType second() const
> +    {
> +	   return m_value & 0xffffffff;
> +    }
> +
> +    bool operator==(const InterferenceEdge32Bit& other) const
> +    {
> +	   return m_value == other.m_value;
> +    }
> +
> +    bool isHashTableDeletedValue() const
> +    {
> +	   return *this == InterferenceEdge32Bit(WTF::HashTableDeletedValue);
> +    }
> +
> +    unsigned hash() const
> +    {
> +	   return WTF::IntHash<uint64_t>::hash(m_value);
> +    }
> +
> +    void dump(PrintStream& out) const
> +    {
> +	   out.print(first(), "<=>", second());
> +    }
> +
> +private:
> +    uint64_t m_value { 0 };
> +};

The implementation looks like very similar. Can we make them templatized one?
Like,

template<typename IndexType, typename PairStorage>
class InterferenceEdge {
    static_assert(sizeof(IndexType) * 2 == sizeof(PairStorage));
    ...
};

using InferenceEdge16 = InterferenceEdge<uint16_t, uint32_t>;
using InferenceEdge32 = InterferenceEdge<uint32_t, uint64_t>;