[PATCH] D26224: NewGVN

[Daniel Berlin via llvm-commits]

On Sun, Nov 27, 2016 at 6:14 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>> ================
>> 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?
>

In particular, If you are thinking of loads/stores, both override
getHashValue.
So, AFAIK, they get the right answers all the time.

It would be nice if everything was nicely commented and consistent here,
however.

When i originally wrote it, I marked it for later cleanup, but it seems
those TODO's and FIXMEs disappeared at some point :(

If you are thinking of the ~1U and ~0U case, these are special keys and
that's just okay.

Notice also that the current GVN.cpp does the same thing with those.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161127/a955fcf5/attachment.html>