GlobalISel: remove assert that memcpy Src and Dst addrspace must be identical

[vtjnash]

Summary

The LangRef does not require these arguments to have the same type.

[vtjnash]

needed for memmove too

[loladiro]

LGTM, from an IR legality perspective, but somebody familiar with GlobalISel should take a look from that perspective - adding authors/reviewers of https://reviews.llvm.org/D65167, which added this code. Also, we should have a test.

[aemerson]

Yes, a test would be good.

[vtjnash]

adding test

[vtjnash]

@aemerson are you happy with this test for approval and merge now?

[aemerson]

LGTM.

[arsenm]

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

7802 ↗

(On Diff #381349)

I think the variable names are now more confusing, this is still a PtrTy. SrcPtrTy?

7813 ↗

(On Diff #381349)

DstPtrTy?

[vtjnash]

Rename LoadTy to SrcTy and StoreTy to DstTy

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

bool CombinerHelper::optimizeMemcpy(MachineInstr &MI, Register Dst,
 
   MachineIRBuilder MIB(MI);
   // Now we need to emit a pair of load and stores for each of the types we've
   // collected. I.e. for each type, generate a load from the source pointer of
   // that type width, and then generate a corresponding store to the dest buffer
   // of that value loaded. This can result in a sequence of loads and stores
   // mixed types, depending on what the target specifies as good types to use.
   unsigned CurrOffset = 0;
-  LLT PtrTy = MRI.getType(Src);
   unsigned Size = KnownLen;
   for (auto CopyTy : MemOps) {
     // Issuing an unaligned load / store pair  that overlaps with the previous
     // pair. Adjust the offset accordingly.
     if (CopyTy.getSizeInBytes() > Size)
       CurrOffset -= CopyTy.getSizeInBytes() - Size;
 
     // Construct MMOs for the accesses.
     auto *LoadMMO =
         MF.getMachineMemOperand(&SrcMMO, CurrOffset, CopyTy.getSizeInBytes());
     auto *StoreMMO =
         MF.getMachineMemOperand(&DstMMO, CurrOffset, CopyTy.getSizeInBytes());
 
     // Create the load.
     Register LoadPtr = Src;
     Register Offset;
     if (CurrOffset != 0) {
-      Offset = MIB.buildConstant(LLT::scalar(PtrTy.getSizeInBits()), CurrOffset)
-                   .getReg(0);
-      LoadPtr = MIB.buildPtrAdd(PtrTy, Src, Offset).getReg(0);
+      LLT LoadTy = MRI.getType(Src);
+      Offset =
+          MIB.buildConstant(LLT::scalar(LoadTy.getSizeInBits()), CurrOffset)
+      LoadPtr = MIB.buildPtrAdd(LoadTy, Src, Offset).getReg(0);
     }
     auto LdVal = MIB.buildLoad(CopyTy, LoadPtr, *LoadMMO);
 
     // Create the store.
-    Register StorePtr =
-        CurrOffset == 0 ? Dst : MIB.buildPtrAdd(PtrTy, Dst, Offset).getReg(0);
+    Register StorePtr = Dst;
+    if (CurrOffset != 0) {
+      LLT StoreTy = MRI.getType(Dst);
+      StorePtr = MIB.buildPtrAdd(StoreTy, Dst, Offset).getReg(0);
+    }
     MIB.buildStore(LdVal, StorePtr, *StoreMMO);
     CurrOffset += CopyTy.getSizeInBytes();
     Size -= CopyTy.getSizeInBytes();
   }
 
   MI.eraseFromParent();
   return true;
 }
bool CombinerHelper::optimizeMemmove(MachineInstr &MI, Register Dst,
 
   LLVM_DEBUG(dbgs() << "Inlining memmove: " << MI << " into loads & stores\n");
 
   MachineIRBuilder MIB(MI);
   // Memmove requires that we perform the loads first before issuing the stores.
   // Apart from that, this loop is pretty much doing the same thing as the
   // memcpy codegen function.
   unsigned CurrOffset = 0;
-  LLT PtrTy = MRI.getType(Src);
   SmallVector<Register, 16> LoadVals;
   for (auto CopyTy : MemOps) {
     // Construct MMO for the load.
     auto *LoadMMO =
         MF.getMachineMemOperand(&SrcMMO, CurrOffset, CopyTy.getSizeInBytes());
 
     // Create the load.
     Register LoadPtr = Src;
     if (CurrOffset != 0) {
+      LLT LoadTy = MRI.getType(Src);
       auto Offset =
-          MIB.buildConstant(LLT::scalar(PtrTy.getSizeInBits()), CurrOffset);
-      LoadPtr = MIB.buildPtrAdd(PtrTy, Src, Offset).getReg(0);
+          MIB.buildConstant(LLT::scalar(LoadTy.getSizeInBits()), CurrOffset);
+      LoadPtr = MIB.buildPtrAdd(LoadTy, Src, Offset).getReg(0);
     }
     LoadVals.push_back(MIB.buildLoad(CopyTy, LoadPtr, *LoadMMO).getReg(0));
     CurrOffset += CopyTy.getSizeInBytes();
   }
 
   CurrOffset = 0;
   for (unsigned I = 0; I < MemOps.size(); ++I) {
     LLT CopyTy = MemOps[I];
     // Now store the values loaded.
     auto *StoreMMO =
         MF.getMachineMemOperand(&DstMMO, CurrOffset, CopyTy.getSizeInBytes());
 
     Register StorePtr = Dst;
     if (CurrOffset != 0) {
+      LLT StoreTy = MRI.getType(Dst);
       auto Offset =
-          MIB.buildConstant(LLT::scalar(PtrTy.getSizeInBits()), CurrOffset);
-      StorePtr = MIB.buildPtrAdd(PtrTy, Dst, Offset).getReg(0);
+          MIB.buildConstant(LLT::scalar(StoreTy.getSizeInBits()), CurrOffset);
+      StorePtr = MIB.buildPtrAdd(StoreTy, Dst, Offset).getReg(0);
     }
     MIB.buildStore(LoadVals[I], StorePtr, *StoreMMO);
     CurrOffset += CopyTy.getSizeInBytes();
   }
   MI.eraseFromParent();
   return true;
 }