[PATCH] D29190: InferAddressSpaces: Support atomics

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:238
   for (Instruction &I : instructions(F)) {
-    if (isa<LoadInst>(I)) {
+    if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
       appendsGenericAddressExpressionToPostorderStack(
----------------
auto*?


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:249
+      appendsGenericAddressExpressionToPostorderStack(
+        CmpX->getPointerOperand(), &PostorderStack, &Visited);
     }
----------------
Would this be better as

  auto push = [&](Value* Ptr) {
    appendsGenericAddressExpressionToPostorderStack(
      Ptr, &PostorderStack, &Visited);
  };
  if (LoadInst *LI = ...)
    push(LI->getPointerOperand());

etc?  As is I have to read closely to see whether or not there's anything interesting going on in these calls.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:537
 
+static bool isCompatiblePtrUse(Use &U) {
+  User *Inst = U.getUser();
----------------
Can we use a different word than "compatible"?  It's not clear obvious what is the indirect object.  A brief comment might also be helpful here.


https://reviews.llvm.org/D29190