[PATCH] D29211: InferAddressSpaces: Support memory intrinsics

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:669
+
+      // Intrinsic users may see the same pointer operand in multiple
+      // operands. Skip to the next instruction.
----------------
arsenm wrote:
> jlebar wrote:
> > It's not just intrinsic users, right?  Regular Instructions could have the same problem?
> > 
> > How would you feel if we restructured this to be
> > 
> >   Use &U = *I;
> >   ++I;
> >   while (I != E && I->getUser() == CurUser) ++I;
> > 
> >   if (updateSimplePtrUse(U) || updateMemIntrinsicUse(U))
> >     continue;
> > 
> >   ...
> > 
> > This seems to be a consistent level of abstraction, instead of having one function which does an update and the other which just does a check and leaves the update to the caller.  And it's also obvious how to extend this code to handle other cases -- just add a new `updateFoo` function to the if statement.
> My definition of "SimplePtrUse" was that it has a single pointer operand that can be trivially replaced, so this was specifically avoiding that problem and treating multi users as the complex case. My patches for icmp/select have that problem, and they are handled separately from updateSimplePtrUse
I understand, but there's no harm in running the loop even in this case, right?

It just seems to me much simpler conceptually to say:

 - first advance the `I` iterator as needed.
 - then muck with the instructions.


https://reviews.llvm.org/D29211