[PATCH] D52294: [InstCombine] Fix incongruous GEP type addrspace

[Ewan Crawford via Phabricator via llvm-commits]

EwanCrawford added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2033-2035
+      Value *NGEP =
+            GEP.isInBounds()
+                ? Builder.CreateInBoundsGEP(nullptr, SrcOp, makeArrayRef(Ops).slice(1))
----------------
lebedev.ri wrote:
> I do not understand, why do we need to create a new GEP here, as compared to the old code?
> Why can't we just only create `AddrSpaceCastInst` if needed?
Thanks for taking a look @lebedev.ri!

Doing `setOperand` then `setSourceElementType` in the old code doesn't actually change the type of the GEP Value for this test case. I get `i32 addrspace(3)*` for `GEP.getType()` both before and after these operation, despite the fact that the GEP now points to a `i32*` afterwards.

Therefore if we call `new addrSpaceCastInst(GEP, GEPType)` we hit an assert since the types of GEP and GEPType are identical. However if we create a new GEP,  then it is correctly instantiated with the `i32*` type and the `addrSpaceCastInst` can be created successfully.

That was my thinking anyway.


Repository:
  rL LLVM

https://reviews.llvm.org/D52294