RFR: 8329194: Cleanup Type::cmp definition and usage

[Christian Hagedorn]

On Thu, 28 Mar 2024 15:13:48 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

> Hi all, this patch aims to cleanup `Type::cmp` by changing it from returning a `0` when types are equal and `1` when they are not, to it returning a boolean denoting equality. This makes its usages at various callsites more intuitive. However, as it is passed to the type dictionary as a comparator, a lambda is needed to map the boolean to a comparison value. 
> 
> I was also considering changing the name to `Type::equals` as it's not really returning a comparison value anymore, but I felt it would be too similar to `Type::eq`. If this would be preferred though, I can change it.
> 
> Tier 1 testing passes on my machine. Reviews and thoughts would be appreciated!

Overall a good improvement and makes it more intuitive. A few comments.

src/hotspot/cpu/x86/x86.ad line 10010:

> 10008:     const MachNode* mask1 = static_cast<const MachNode*>(this->in(this->operand_index($src1)));
> 10009:     const MachNode* mask2 = static_cast<const MachNode*>(this->in(this->operand_index($src2)));
> 10010:     assert(Type::cmp(mask1->bottom_type(), mask2->bottom_type()), "");

While at it, you could add a message like "should be false" for good practice.

src/hotspot/share/opto/node.cpp line 3014:

> 3012: }
> 3013: bool TypeNode::cmp(const Node& n) const {
> 3014:   return Type::cmp(_type, ((TypeNode&)n)._type);

While at it, you can replace the cast with `as_Type()`:
Suggestion:

  return Type::cmp(_type, (n.as_Type()->_type);

src/hotspot/share/opto/type.cpp line 444:

> 442:   };
> 443: 
> 444:   _shared_type_dict = new (shared_type_arena) Dict((CmpKey) type_cmp, (Hash) Type::uhash, shared_type_arena, 128);

Couldn't you make `type_cmp` a `CmpKey` instead of `auto` and then remove the cast here? On the other hand, you could probably also just remove the `CmpKey` cast here but it might be more explicit when also changing the lambda type.

src/hotspot/share/opto/type.hpp line 219:

> 217:   static const Type *make(enum TYPES);
> 218:   // Test for equivalence of types
> 219:   static int cmp( const Type *const t1, const Type *const t2 );

Was it required to remove the `consts` here?

-------------

PR Review: https://git.openjdk.org/jdk/pull/18533#pullrequestreview-2000181356
PR Review Comment: https://git.openjdk.org/jdk/pull/18533#discussion_r1565282766
PR Review Comment: https://git.openjdk.org/jdk/pull/18533#discussion_r1565287006
PR Review Comment: https://git.openjdk.org/jdk/pull/18533#discussion_r1565290581
PR Review Comment: https://git.openjdk.org/jdk/pull/18533#discussion_r1565293370