[PATCH] D29213: InferAddressSpaces: Handle icmp

[Justin Lebar via Phabricator via llvm-commits]

jlebar accepted this revision.
jlebar added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:759
+            if (KOtherSrc->getType()->getPointerAddressSpace() == NewAS ||
+                isa<UndefValue>(KOtherSrc)) {
+              Cmp->setOperand(SrcIdx, NewV);
----------------
arsenm wrote:
> 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
sgtm


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:744
+          //   %cmp = icmp eq float* %p, %q
+          // if both p and q are inferred to be shared, we can rewrite %cmp as
+          //   %cmp = icmp eq float addrspace(3)* %new_p, %new_q
----------------
Since this is now generic, we probably shouldn't say "shared" here.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:749
+          //   %generic_q = addrspacecast float addrspace(3)* %new_q to float*
+          //   %cmp = icmp eq float* %generic_p, %generic_q
+
----------------
Is this comment now out of date?  I'm confused about "instead of currently".

Would also prefer for the comment to start with prose.  For example:

  // If we can infer that both pointers are in the same addrspace, transform e.g.
  //   %cmp = icmp eq float*%p, %q
  // into
  //   %cmp = icmp eq float addrspace(3)* %new_p, %new_q

Is that all we need to say here?


https://reviews.llvm.org/D29213