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

[James Price via Phabricator via llvm-commits]

jprice added a comment.

Thanks for the feedback, responses inline.



================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:445
+  void replacePtrUsesAndPropagateAddrSpace(Value *OldPtr, Value *NewPtr) {
+    unsigned NewAddrSpace = NewPtr->getType()->getPointerAddressSpace();
+
----------------
mehdi_amini wrote:
> Why not shortcut here:
> 
> ```
> If (OldPtr->getType()->getPointerAddressSpace() == NewAddrSpace) {
>   replaceInstUsesWith(OldPtr, NewPtr);
>   return;
> }
> ```
This breaks certain cases where `OldPtr` is the source to an `addrspacecast` which may result in casting `NewPtr` to an address space that it shouldn't. 

We can however shortcut inside the loop over users, as long as we handle `addrspacecast` instructions first. I've done this in the latest patch.


================
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
----------------
mehdi_amini wrote:
> Shouldn't this be `if (NewPtr->getType() == ...`?
> (If so, you're missing a test to cover this)
You're right.

There is a test that covers this case (test11), but since this check just results in a shortcut, it was passing anyway (by introducing an unnecessary NOP cast which was nuked anyway).



================
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) {
----------------
mehdi_amini wrote:
> 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.
> 
It does indeed block the transformation, unless we //know// we can use an `addrspacecast` because there is already one being used on the global when it is passed as a source to the memcpy. I've added a comment here to make this assumption clear.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:101
+        if (IsArgOperand && !isa<MemTransferInst>(I))
+          ArgAddrSpaces.emplace_back(U->getType()->getPointerAddressSpace());
+
----------------
mehdi_amini wrote:
> 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?
I'm not sure I see how this would work. There are either one or two valid address spaces that can be used for function arguments (the source and raw source of the memory transfer that we are trying to remove). We don't know whether we will see the memory transfer first, or the function calls that use the alloca.


https://reviews.llvm.org/D28300