[PATCH] D28300: [InstCombine] Fix address space handling when removing allocas

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

A few inline comments



================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:442
 
+  /// replacePtrUsesAndPropagateAddrSpace - Recursively update the users of a
+  /// pointer with a new value, propagating the new address space to all users.
----------------
Nit: Don't repeat the name of the function in the comment, we use to do this but we changed the doxygen settings some time ago and this is deprecated.


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:444
+  /// pointer with a new value, propagating the new address space to all users.
+  void replacePtrUsesAndPropagateAddrSpace(Value *OldPtr, Value *NewPtr) {
+    unsigned NewAddrSpace = NewPtr->getType()->getPointerAddressSpace();
----------------
Can you turn this into a work-queue iterative loop? I'm worried about unbounded recursion.


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:445
+  void replacePtrUsesAndPropagateAddrSpace(Value *OldPtr, Value *NewPtr) {
+    unsigned NewAddrSpace = NewPtr->getType()->getPointerAddressSpace();
+
----------------
Why not shortcut here:

```
If (OldPtr->getType()->getPointerAddressSpace() == NewAddrSpace) {
  replaceInstUsesWith(OldPtr, NewPtr);
  return;
}
```


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:448
+    SmallVector<Value *, 8> Users(OldPtr->users().begin(),
+                                  OldPtr->users().end());
+    for (auto U = Users.begin(); U != Users.end(); U++) {
----------------
Add a comment why you need to copy?


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:449
+                                  OldPtr->users().end());
+    for (auto U = Users.begin(); U != Users.end(); U++) {
+      if (auto BI = dyn_cast<BitCastInst>(*U)) {
----------------
I haven't figured why it isn't a for-range loop?


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:457
+      } else if (auto ASCI = dyn_cast<AddrSpaceCastInst>(*U)) {
+        if (OldPtr->getType() == ASCI->getType()) {
+          // If the type already matches, remove the cast completely
----------------
Shouldn't this be `if (NewPtr->getType() == ...`?
(If so, you're missing a test to cover this)


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:479
+            new LoadInst(NewPtr, "", LI->isVolatile(), LI->getAlignment(), LI);
+        LI->replaceAllUsesWith(NewLoad);
+      } else if (auto MI = dyn_cast<MemTransferInst>(*U)) {
----------------
Is it needed for loads? (Maybe because of typeless pointers?)


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:499
+
+        // If the address spaces don't match, cast the source so that they do.
+        if (OldPtr->getType()->getPointerAddressSpace() != NewAddrSpace) {
----------------
I'm worried about the validity of this. I would expect passing a pointer to a function to block this transformation when the address space mismatch.



================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:505
+            Src = new AddrSpaceCastInst(NewPtr, OldPtr->getType(), "", CI);
+          }
+        }
----------------
Nit: Do not use braces for trivial blocks.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:62
 
+  SmallVector<unsigned, 8> ArgAddrSpaces;
+
----------------
Document it.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:101
+        if (IsArgOperand && !isa<MemTransferInst>(I))
+          ArgAddrSpaces.emplace_back(U->getType()->getPointerAddressSpace());
+
----------------
It seems that you will return false if there is any mismatch (line 168).

Do we need a vector? Can't we just keep one AS and return false as soon as we find a mismatch here?


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:163
+  // Check that the address spaces of all function call arguments match the
+  // source of the memory transfer
+  if (TheCopy) {
----------------
Nit: period to end sentence.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:312
           eraseInstFromFunction(*ToDelete[i]);
+
         Constant *TheSrc = cast<Constant>(Copy->getSource());
----------------
Spurious change


https://reviews.llvm.org/D28300