[PATCH] D82892: [NFC] Added comparision for all types in haveSameSpecialState() of Instruction.cpp

[Vishal Chebrolu via Phabricator via llvm-commits]

vish99 added a comment.

In D82892#2208245 <https://reviews.llvm.org/D82892#2208245>, @jfb wrote:

> I didn't check that all of these compare all the required state for all opcodes.
>
> That being said, this still seems somewhat brittle, if less so than before. Are there sufficient tests to find future divergences?

When a new instruction has been added and the compareSpecialState() method has not been updated, it leads to failure. But when there is an addition of a new operand, it's the responsibility of the author to change the compareInstSpecificProperties() method of that method(which can be done easily as it'll be in the same file) and this can update all the comparators. Currently, I don't think there are enough tests.



================
Comment at: llvm/lib/IR/Instructions.cpp:161
+  case Type::PointerTyID:
+    assert(PTyL && PTyR && "Both types must be pointers here.");
+    return compareIntegers(PTyL->getAddressSpace(), PTyR->getAddressSpace());
----------------
nikic wrote:
> The compareTypes() function looks a bit problematic to me. The issue is that MergeFunctions considers integer types and all pointer types of the same size equal. Other places in LLVM require that the type must be exactly identical. The function as implemented here seems to go some middle way, where pointers are not considered equal to integers, but all pointer types are the same. That's in the spirit of opaque pointers, but we're not there yet.
How about we also compare the element types of the pointers through a recursive call to compareTypes() along with the Address space comparison?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82892/new/

https://reviews.llvm.org/D82892