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

178

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 {
   // 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;
 }
 
 namespace {
 
 class MemsetRanges {
   using range_iterator = SmallVectorImpl<MemsetRange>::iterator;
 
   /// A sorted list of the memset ranges.
   SmallVector<MemsetRange, 8> Ranges;
 
   const DataLayout &DL;
 
 public:
   MemsetRanges(const DataLayout &DL) : DL(DL) {}
 
   using const_iterator = SmallVectorImpl<MemsetRange>::const_iterator;
 
   const_iterator begin() const { return Ranges.begin(); }
   const_iterator end() const { return Ranges.end(); }
   bool empty() const { return Ranges.empty(); }
 
   void addInst(int64_t OffsetFromFirst, Instruction *Inst) {
     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);
+    unsigned Align = SI->getAlignment();
+    if (!Align)
+      Align = DL.getABITypeAlignment(SI->getOperand(0)->getType());
+    addRange(OffsetFromFirst, StoreSize, SI->getPointerOperand(), Align, 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,
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());
 

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)
+}