[PATCH] D29213: InferAddressSpaces: Handle icmp

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In https://reviews.llvm.org/D29213#658940, @jlebar wrote:

> Just so I know how to read these tests: AS(4) is the flat AS?


Yes



================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:759
+            if (KOtherSrc->getType()->getPointerAddressSpace() == NewAS ||
+                isa<UndefValue>(KOtherSrc)) {
+              Cmp->setOperand(SrcIdx, NewV);
----------------
jlebar wrote:
> Can we have a test for `icmp x, undef` and `icmp undef, x`?
I can add another test for undef, x, but there's no point in trying to handle these in the code since these are canonicalized to constants in the RHS


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:762
+              Cmp->setOperand(OtherIdx,
+                ConstantExpr::getAddrSpaceCast(KOtherSrc, NewV->getType()));
+              continue;
----------------
jlebar wrote:
> If KOtherSrc->AS() == NewAS, then isn't this ASC redundant?  It seems we should only need it if KOtherSrc is undef, and in that case, can we just substitute a new undef value, instead of ASC'ing the old undef?
I made this too conservative by adding the new check here, so this is backwards. The addrspace check is redundant with the cast. We need to cast the constants, otherwise it's making an assumption about the bit values of the pointer. For undef I think replacing the type directly would be OK, but the ConstantExpr cast should do the right thing (and I think this will be necessary in the future with freeze anyway)


https://reviews.llvm.org/D29213