[PATCH] D38789: MachineInstr: Force isDef to match in isIdenticalTo

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

In https://reviews.llvm.org/D38789#894648, @kparzysz wrote:

> Here's a comment from `include/llvm/CodeGen/MachineInstr.h` (definition of `MachineInstrExpressionTrait`)
>
>   /// Special DenseMapInfo traits to compare MachineInstr* by *value* of the
>   /// instruction rather than by pointer value.
>   /// The hashing and equality testing functions ignore definitions so this is
>   /// useful for CSE, etc.
>
>
> The `isEqual` function explicitly requests `IgnoreVRegDefs`, supposedly on the basis that `getHashValue` would do the same.  Your observation is that this is not the case.  Assuming that this comment was true at some point, it may be worthwhile to check how the deviation was introduced.


Well, they ignore "proper" definitions (e.g. the %1114 vs %1115 in my example). That's a def on both instructions. But the last operand is only a def in one of the instructions, not both, so it's less clear what should happen. I tend to agree with the hashing function, which says that they are different.

Now that you mention it, I suppose they only need to be different if Check == IgnoreVRegDefs, since that is what the hashing function assumes, and only for non-virtual registers (physical and sentries). I'll update the patch to that effect.

I think in light of that idea, we should have some tests making sure that MachineInstrExpressionTrait::isEqual and getHashFunction agree about things (independent of the tests for MachineInstr::isIdenticalTo).


https://reviews.llvm.org/D38789