[MemCpyOpt] Relax libcall checks

[nikic]

Summary

Rather than blocking the whole MemCpyOpt pass if the libcalls are not available, only disable creation of new memset/memcpy intrinsics where none existed previously. This only affects the store merging and load-store conversion optimization. Other optimizations are derived from existing intrinsics, which are well-defined in the absence of libcalls -- not having the libcalls just means that call simplification won't convert them to intrinsics.

This is a weaker variation of D104801, which dropped these checks entirely.

[nikic]

Add test

[tra]

LGTM in general.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

677

I'd add a TODO/FIXME. The only reason we need to do it is because of the assumption in some places (sanitizeers and some downstream back-ends) that we can't handle LLVM intrinsic w/o the matching libcall. Materializing LLVM intrinsics is not illegal and other passes do it.

803

Ditto.

llvm/test/Transforms/MemCpyOpt/no-libcalls.ll

3 ↗

(On Diff #361512)

This is a bit ironic as AMDGPU back-end (as well as NVPTX) *can* actually handle memory intrinsics w/o the matching libcalls.

https://godbolt.org/z/Koedbzn5x

I think it may make sense to fold D106401 into this patch and add another test for AMDGPU + all optimizations enabled by the flag.

[asbirlea]

Just my thoughts, I may not have all the info here..

I think materializing memcpy and memset should be standard and not be an issue in either sanitizers or downstream backends. There is a very small subset of intrinsics where I'd expect this, where passes introducing them are already in the default pipeline (e.g. MemCpyOpt, LoopIdiomRecognize).
I also understand the push back on the reverted patch, in that it was changing the status quo as far as expectations and a standardization should be done with proper RFC and llvm-dev discussions. I just think that this is something to consider changing in the long run.

This patch and the flag in D106401 makes sense to resolve the issues for the NVPTX use case (and AMDGPU as well I assume).

llvm/test/Transforms/MemCpyOpt/no-libcalls.ll

3 ↗

(On Diff #361512)

Either folding into this or as a separate patch is reasonable IMO.
@tra: do you have another test that would be relevant to add in D106401 for nvptx (beside an additional RUN line in this one with the flag I mean)?

[tra]

llvm/test/Transforms/MemCpyOpt/no-libcalls.ll

3 ↗

(On Diff #361512)

If the flag is folded into this patch, we should have a test for it (see my comment above). I'll add it to D106401 if it's not folded in.

[tra]

@nikic So, what is your plan for this patch? I'm fine with it whether it's combined with D106401 or not.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

if (DL.isNonIntegralPointerType(StoredVal->getType()->getScalarType()))
     return false;
 
   // Load to store forwarding can be interpreted as memcpy.
   if (LoadInst *LI = dyn_cast<LoadInst>(StoredVal)) {
     if (LI->isSimple() && LI->hasOneUse() &&
         LI->getParent() == SI->getParent()) {
 
       auto *T = LI->getType();
-      if (T->isAggregateType()) {
+      // Don't introduce calls to memcpy/memmove intrinsics out of thin air if
+      // the corresponding libcalls are not available.
+      if (T->isAggregateType() && TLI->has(LibFunc_memcpy) &&
+          TLI->has(LibFunc_memmove)) {
         MemoryLocation LoadLoc = MemoryLocation::get(LI);
 
         // We use alias analysis to check if an instruction may store to
         // the memory we load from in between the load and the store. If
         // such an instruction is found, we try to promote there instead
         // of at the store position.
         // TODO: Can use MSSA for this.
         Instruction *P = SI;
if (LI->isSimple() && LI->hasOneUse() &&
           eraseInstruction(LI);
           ++NumMemCpyInstr;
           return true;
         }
       }
     }
   }
 
+  // The following code creates memset intrinsics out of thin air. Don't do
+  // this if the corresponding libfunc is not available.
+  if (!TLI->has(LibFunc_memset))
+    return false;
+
   // There are two cases that are interesting for this code to handle: memcpy
   // and memset.  Right now we only handle memset.
 
   // Ensure that the value being stored is something that can be memset'able a
   // byte at a time like "0" or "-1" or any width, as well as things like
   // 0xA0A0A0A0 and 0.0.
   auto *V = SI->getOperand(0);
   if (Value *ByteVal = isBytewiseValue(V, DL)) {
bool MemCpyOptPass::processMemCpy(MemCpyInst *M, BasicBlock::iterator &BBI) {
   }
 
   return false;
 }
 
 /// Transforms memmove calls to memcpy calls when the src/dst are guaranteed
 /// not to alias.
 bool MemCpyOptPass::processMemMove(MemMoveInst *M) {
-  if (!TLI->has(LibFunc_memmove))
-    return false;
-
   // See if the pointers alias.
   if (!AA->isNoAlias(MemoryLocation::getForDest(M),
                      MemoryLocation::getForSource(M)))
     return false;
 
   LLVM_DEBUG(dbgs() << "MemCpyOptPass: Optimizing memmove -> memcpy: " << *M
                     << "\n");
 
bool MemCpyOptPass::runImpl(Function &F, MemoryDependenceResults *MD_,
   MD = MD_;
   TLI = TLI_;
   AA = AA_;
   AC = AC_;
   DT = DT_;
   MSSA = MSSA_;
   MemorySSAUpdater MSSAU_(MSSA_);
   MSSAU = MSSA_ ? &MSSAU_ : nullptr;
-  // If we don't have at least memset and memcpy, there is little point of doing
-  // anything here.  These are required by a freestanding implementation, so if
-  // even they are disabled, there is no point in trying hard.
-  if (!TLI->has(LibFunc_memset) || !TLI->has(LibFunc_memcpy))
-    return false;
 
   while (true) {
     if (!iterateOnFunction(F))
       break;
     MadeChange = true;
   }
 
   if (MSSA_ && VerifyMemorySSA)