[PATCH] D29211: InferAddressSpaces: Support memory intrinsics

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:568
 
+/// Handle updating uses that require more complex processing than simply
+/// replacing an operand. e.g. intrinsic uses that need to be re-mangled.
----------------
Instead of "handle updating uses", can we just say "Updates uses"?


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:571
+/// \returns true on sucess but does not remove the user instruction \p U.
+static bool handleComplexPtrUse(User &U, Value *OldV, Value *NewV) {
+  if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(&U)) {
----------------
How about we call this updateMemIntrinsicUse, or updateIfIsMemIntrinsic?  We can add other stuff to other functions.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:572
+static bool handleComplexPtrUse(User &U, Value *OldV, Value *NewV) {
+  if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(&U)) {
+    if (MI->isVolatile())
----------------
The nesting here, and particularly the llvm_unreachable after the `else` below, is pretty hard for me to read.

Could we restructure the control flow as

  auto *MI = dyn_cast<MemIntrinsic>(&U);
  if (!MI || MI->isVolatile())
    return false;
  
  ...

?


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:582
+
+    if (MemSetInst *MSI = dyn_cast<MemSetInst>(MI)) {
+      NewCall = B.CreateMemSet(NewV, MSI->getValue(),
----------------
auto* with dyn_cast, here and elsewhere?


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:585
+                               MSI->getLength(), MSI->getAlignment(),
+                               false, TBAA, ScopeMD, NoAliasMD);
+    } else if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(MI)) {
----------------
Would you mind adding `/*foo=*/false` here and below?  See my now five-year-old screed, http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:587
+    } else if (MemTransferInst *MTI = dyn_cast<MemTransferInst>(MI)) {
+      // Be careful in case this is a self to self copy.
+      Value *Src = MTI->getRawSource();
----------------
Nit: self-to-self


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:669
+
+      // Intrinsic users may see the same pointer operand in multiple
+      // operands. Skip to the next instruction.
----------------
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.


https://reviews.llvm.org/D29211