[PATCH] D26224: NewGVN

[Daniel Berlin via llvm-commits]

>
>
> ================
> Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:71-83
> +  bool operator==(const Expression &Other) const {
> +    if (Opcode != Other.Opcode)
> +      return false;
> +    if (Opcode == ~0U || Opcode == ~1U)
> +      return true;
> +    // Compare etype for anything but load and store.
> +    if (getExpressionType() != ExpressionTypeLoad &&
> ----------------
> The behavior of this operator is quite nonintuitive. It's actually
> `isTrivallyCongruent()`, not is equal. However if this is the natural
> equality definition for this type, then it would be fine with a comment
> explaining that.
>
>
This is deliberate, so yes.


>
> ================
> Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:87-89
> +  virtual hash_code getHashValue() const {
> +    return hash_combine(EType, Opcode);
> +  }
> ----------------
> This can return a different `hash_code` for Expressions which compare
> equal.
>

What example are you thinking of?

> ================
> Comment at: lib/Transforms/Scalar/NewGVN.cpp:145
> +        return false;
> +      return *LHS == *RHS;
> +    }
> ----------------
> This seems like the wrong equality operator for the above hash. It will
> return true for things that don't hash to the same value.
>
> Again, example?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161127/44c2ea8b/attachment.html>