MemCpyOpt cannot use ABI alignment even if it was not given

[aqjune]

Summary

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=44388 which incorrectly assigns an ABI alignment to memset when there was no explicit alignment given.

[nikic]

define void @test(i32* nocapture %P) nounwind ssp {
entry:
  store i32 0, i32* %P
  %add.ptr = getelementptr inbounds i32, i32* %P, i64 1
  %0 = bitcast i32* %add.ptr to i8*
  tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false)
  ret void
}

declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg)

This example no longer preserves the alignment (align 4 from ABI alignment on the store) after your patch. Possibly you need to capture that when populating Range.Alignment.

[aqjune]

• Let MemsetRanges::addStore explicitly record ABI alignment

[nikic]

LG

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

196

I'd suggest moving the findStoreAlignment() helper below higher up in the file and use that, as it implements the same logic.

[aqjune]

• Move find*Alignment up to the code & use it

[gchatelet]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

149

I believe this can now be written as

return DL.getValueOrABITypeAlignment(
    SI->getAlign(),
    SI->getOperand(0)->getType());

154

ditto

[aqjune]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

149

Fixed via https://github.com/llvm/llvm-project/commit/5687acf431de4f6a156b2f6906eb4eb503ac70ce

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

bool MemsetRange::isProfitableToUseMemset(const DataLayout &DL) const {
   unsigned NumByteStores = Bytes % MaxIntSize;
 
   // If we will reduce the # stores (according to this heuristic), do the
   // transformation.  This encourages merging 4 x i8 -> i32 and 2 x i16 -> i32
   // etc.
   return TheStores.size() > NumPointerStores+NumByteStores;
 }
 
+
+static Align findStoreAlignment(const DataLayout &DL, const StoreInst *SI) {
+  return DL.getValueOrABITypeAlignment(MaybeAlign(SI->getAlignment()),
+                                       SI->getOperand(0)->getType());
+}
+
+static Align findLoadAlignment(const DataLayout &DL, const LoadInst *LI) {
+  return DL.getValueOrABITypeAlignment(MaybeAlign(LI->getAlignment()),
+                                       LI->getType());
+}
+
+static Align findCommonAlignment(const DataLayout &DL, const StoreInst *SI,
+                                 const LoadInst *LI) {
+  Align StoreAlign = findStoreAlignment(DL, SI);
+  Align LoadAlign = findLoadAlignment(DL, LI);
+  return commonAlignment(StoreAlign, LoadAlign);
+}
+
 namespace {
 
 class MemsetRanges {
   using range_iterator = SmallVectorImpl<MemsetRange>::iterator;
 
   /// A sorted list of the memset ranges.
   SmallVector<MemsetRange, 8> Ranges;
 
if (StoreInst *SI = dyn_cast<StoreInst>(Inst))
       addStore(OffsetFromFirst, SI);
     else
       addMemSet(OffsetFromFirst, cast<MemSetInst>(Inst));
   }
 
   void addStore(int64_t OffsetFromFirst, StoreInst *SI) {
     int64_t StoreSize = DL.getTypeStoreSize(SI->getOperand(0)->getType());
 
-    addRange(OffsetFromFirst, StoreSize,
-             SI->getPointerOperand(), SI->getAlignment(), SI);
+    addRange(OffsetFromFirst, StoreSize, SI->getPointerOperand(),
+             findStoreAlignment(DL, SI).value(), SI);
   }
 
   void addMemSet(int64_t OffsetFromFirst, MemSetInst *MSI) {
     int64_t Size = cast<ConstantInt>(MSI->getLength())->getZExtValue();
     addRange(OffsetFromFirst, Size, MSI->getDest(), MSI->getDestAlignment(), MSI);
   }
 
   void addRange(int64_t Start, int64_t Size, Value *Ptr,
                 unsigned Alignment, Instruction *Inst);
for (const MemsetRange &Range : Ranges) {
     // If it is profitable to lower this range to memset, do so now.
     if (!Range.isProfitableToUseMemset(DL))
       continue;
 
     // Otherwise, we do want to transform this!  Create a new memset.
     // Get the starting pointer of the block.
     StartPtr = Range.StartPtr;
 
-    // Determine alignment
-    const Align Alignment = DL.getValueOrABITypeAlignment(
-        MaybeAlign(Range.Alignment),
-        cast<PointerType>(StartPtr->getType())->getElementType());
-
     AMemSet = Builder.CreateMemSet(StartPtr, ByteVal, Range.End - Range.Start,
-                                   Alignment);
+                                   MaybeAlign(Range.Alignment));
     LLVM_DEBUG(dbgs() << "Replace stores:\n"; for (Instruction *SI
                                                    : Range.TheStores) dbgs()
                                               << *SI << '\n';
                dbgs() << "With: " << *AMemSet << '\n');
 
     if (!Range.TheStores.empty())
       AMemSet->setDebugLoc(Range.TheStores[0]->getDebugLoc());
 
     // Zap all the stores.
     for (Instruction *SI : Range.TheStores) {
       MD->removeInstruction(SI);
       SI->eraseFromParent();
     }
     ++NumMemSetInfer;
   }
 
   return AMemSet;
 }
 
-static Align findStoreAlignment(const DataLayout &DL, const StoreInst *SI) {
-  return DL.getValueOrABITypeAlignment(MaybeAlign(SI->getAlignment()),
-                                       SI->getOperand(0)->getType());
-}
-
-static Align findLoadAlignment(const DataLayout &DL, const LoadInst *LI) {
-  return DL.getValueOrABITypeAlignment(MaybeAlign(LI->getAlignment()),
-                                       LI->getType());
-}
-
-static Align findCommonAlignment(const DataLayout &DL, const StoreInst *SI,
-                                 const LoadInst *LI) {
-  Align StoreAlign = findStoreAlignment(DL, SI);
-  Align LoadAlign = findLoadAlignment(DL, LI);
-  return commonAlignment(StoreAlign, LoadAlign);
-}
-
 // This method try to lift a store instruction before position P.
 // It will lift the store and its argument + that anything that
 // may alias with these.
 // The method returns true if it was successful.
 static bool moveUp(AliasAnalysis &AA, StoreInst *SI, Instruction *P,
                    const LoadInst *LI) {
   // If the store alias this position, early bail out.
   MemoryLocation StoreLoc = MemoryLocation::get(SI);

llvm/test/Transforms/MemCpyOpt/form-memset.ll

 entry:
   %0 = bitcast i32* %P to i8*
   tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 12, i1 false)
   %add.ptr = getelementptr inbounds i32, i32* %P, i64 3
   %1 = bitcast i32* %add.ptr to i8*
   tail call void @llvm.memset.p0i8.i64(i8* %1, i8 0, i64 12, i1 false)
   ret void
 ; CHECK-LABEL: @test6(
-; CHECK: call void @llvm.memset.p0i8.i64(i8* align 4 %2, i8 0, i64 24, i1 false)
+; CHECK: call void @llvm.memset.p0i8.i64(i8* %2, i8 0, i64 24, i1 false)
 }
 
 ; More aggressive heuristic
 ; rdar://9892684
 define void @test7(i32* nocapture %c) nounwind optsize {
   store i32 -1, i32* %c, align 4
   %1 = getelementptr inbounds i32, i32* %c, i32 1
   store i32 -1, i32* %1, align 4

 
 ; PR19092
 define void @test10(i8* nocapture %P) nounwind {
   tail call void @llvm.memset.p0i8.i64(i8* %P, i8 0, i64 42, i1 false)
   tail call void @llvm.memset.p0i8.i64(i8* %P, i8 0, i64 23, i1 false)
   ret void
 ; CHECK-LABEL: @test10(
 ; CHECK-NOT: memset
-; CHECK: call void @llvm.memset.p0i8.i64(i8* align 1 %P, i8 0, i64 42, i1 false)
+; CHECK: call void @llvm.memset.p0i8.i64(i8* %P, i8 0, i64 42, i1 false)
 ; CHECK-NOT: memset
 ; CHECK: ret void
 }
 
 ; Memset followed by odd store.
 define void @test11(i32* nocapture %P) nounwind ssp {
 entry:
   %add.ptr = getelementptr inbounds i32, i32* %P, i64 3
   %0 = bitcast i32* %add.ptr to i8*
   tail call void @llvm.memset.p0i8.i64(i8* %0, i8 1, i64 11, i1 false)
   %arrayidx = getelementptr inbounds i32, i32* %P, i64 0
   %arrayidx.cast = bitcast i32* %arrayidx to i96*
   store i96 310698676526526814092329217, i96* %arrayidx.cast, align 4
   ret void
 ; CHECK-LABEL: @test11(
 ; CHECK-NOT: store
 ; CHECK: call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 1, i64 23, i1 false)
 }
+
+; Alignment should be preserved when there is a store with default align
+define void @test12(i32* nocapture %P) nounwind ssp {
+entry:
+  store i32 0, i32* %P
+  %add.ptr = getelementptr inbounds i32, i32* %P, i64 1
+  %0 = bitcast i32* %add.ptr to i8*
+  tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false)
+  ret void
+; CHECK-LABEL: @test12(
+; CHECK-NOT: store
+; CHECK: call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 0, i64 15, i1 false)
+}