LSV: Fix incorrectly increasing alignment

[arsenm]

Summary

If the unaligned access has a dynamic offset, it may be odd which
would make the adjusted alignment incorrect to use.

[efriedma]

Is there some reason you're not using getOrEnforceKnownAlignment here?

[arsenm]

In D23906#526422, @efriedma wrote:

Is there some reason you're not using getOrEnforceKnownAlignment here?

I didn't know about that one. Switching to that mostly works, except it misses the case (e.g. store_unknown_offset_align1_i32) where it should be OK because the indexing should avoid an unaligned offset because it uses stripPointerCasts instead of GetUnderlyingObject

[arsenm]

Switch to using getOrEnforceKnownAlignment, even though this regresses the case. Maybe it should be extended to handle this case

[efriedma]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

829

I'm sort of confused about what you're doing here... what is StackAdjustedAlignment supposed to be?

[arsenm]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

829

It's supposed to be a normal "good" stack alignment value that will generally allow vectorization. It's a hardcoded 4 because the GPUs we care about this always works. This should be something smarter, but that's a separate change

[efriedma]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

829

Okay... but if StackAdjustedAlignment is the alignment which allows vectorization, shouldn't you be checking "NewAlign <= StackAdjustedAlignment", as opposed to "NewAlign <= Alignment"?

[jlebar]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

830

getOrEnforceKnownAlignment returns 0 on failure, maybe we should check for that instead? (If you want, you could also assert that NewAlign is either 0 or >= StackAdjustedAlignment.)

1057–1058

Why this change? It's equivalent to the old code assuming the alignment is a power of two (which, if we depend on this, we should assert?), but I don't see why we need to make the change at all.

[arsenm]

Address comments

[jlebar]

In D23906#537736, @arsenm wrote:

Address comments

It looks like you only addressed one of the two comments I left. efriedma's comment is also still outstanding afaict. Was that intentional?

[arsenm]

Apparently I never saved the file

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

830

It doesn't actually return 0 on failure, it returns the known alignment. In the motivating testcase, load_unknown_offset_align1_i8, it returns 1 when it can't find the constant offset

[jlebar]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

830

I looked through the code and agree that it is as you describe.

It may be worth fixing the comment in Local.h.

[arsenm]

r281110

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

private:
   ///
   /// The elements of \p Chain must be all loads or all stores and must be in
   /// address order.
   ArrayRef<Instruction *> getVectorizablePrefix(ArrayRef<Instruction *> Chain);
 
   /// Collects load and store instructions to vectorize.
   std::pair<InstrListMap, InstrListMap> collectInstructions(BasicBlock *BB);
 
+  /// See if pointer \p V is based on an an alloca that can have its alignment
+  /// increased. \returns the alloca which can be modified, otherwise null.
+  AllocaInst *canAdjustAllocaAlignment(Value *V, unsigned EltSize,
+                                       unsigned Align);
+
   /// Processes the collected instructions, the \p Map. The values of \p Map
   /// should be all loads or all stores.
   bool vectorizeChains(InstrListMap &Map);
 
   /// Finds the load/stores to consecutive memory addresses and vectorizes them.
   bool vectorizeInstructions(ArrayRef<Instruction *> Instrs);
 
   /// Vectorizes the load instructions in Chain.
else
       Vectorized = vectorizeStoreChain(Operands, &InstructionsProcessed);
 
     Changed |= Vectorized;
   }
 
   return Changed;
 }
 
+AllocaInst *Vectorizer::canAdjustAllocaAlignment(Value *BasePtr,
+                                                 unsigned EltSize,
+                                                 unsigned Align) {
+  // If we're storing to an object on the stack, we control its alignment, so we
+  // can cheat and change it!
+  if (EltSize % StackAdjustedAlignment == 0) {
+    // If the element size is a multiple of the stack element alignment, we can
+    // always change the alignment.
+
+    Value *V = GetUnderlyingObject(BasePtr, DL);
+    return dyn_cast<AllocaInst>(V);
+  }
+
+  // If the element size is smaller than the natural stack alignment, we need to
+  // make sure an odd dynamic offset isn't being added.
+  int64_t Offset = 0;
+  Value *V = GetPointerBaseWithConstantOffset(BasePtr, Offset, DL);
+  if (AllocaInst *AI = dyn_cast<AllocaInst>(V)) {
+    if (Offset % StackAdjustedAlignment == 0)
+      return AI;
+  }
+
+  return nullptr;
+}
+
 bool Vectorizer::vectorizeStoreChain(
     ArrayRef<Instruction *> Chain,
     SmallPtrSet<Instruction *, 16> *InstructionsProcessed) {
   StoreInst *S0 = cast<StoreInst>(Chain[0]);
 
   // If the vector has an int element, default to int for the whole load.
   Type *StoreTy;
   for (Instruction *I : Chain) {
bool Vectorizer::vectorizeStoreChain(
   }
 
   // Update Chain to the valid vectorizable subchain.
   Chain = NewChain;
   ChainSize = Chain.size();
 
   // Store size should be 1B, 2B or multiple of 4B.
   // TODO: Target hook for size constraint?
-  unsigned SzInBytes = (Sz / 8) * ChainSize;
+  unsigned EltSzInBytes = Sz / 8;
+  unsigned SzInBytes = EltSzInBytes * ChainSize;
   if (SzInBytes > 2 && SzInBytes % 4 != 0) {
     DEBUG(dbgs() << "LSV: Size should be 1B, 2B "
                     "or multiple of 4B. Splitting.\n");
     if (SzInBytes == 3)
       return vectorizeStoreChain(Chain.slice(0, ChainSize - 1),
                                  InstructionsProcessed);
 
     auto Chains = splitOddVectorElts(Chain, Sz);
bool Vectorizer::vectorizeStoreChain(
   // Check alignment restrictions.
   unsigned Alignment = getAlignment(S0);
 
   // If the store is going to be misaligned, don't vectorize it.
   if (accessIsMisaligned(SzInBytes, AS, Alignment)) {
     if (S0->getPointerAddressSpace() != 0)
       return false;
 
-    // If we're storing to an object on the stack, we control its alignment,
-    // so we can cheat and change it!
-    Value *V = GetUnderlyingObject(S0->getPointerOperand(), DL);
-    if (AllocaInst *AI = dyn_cast_or_null<AllocaInst>(V)) {
-      AI->setAlignment(StackAdjustedAlignment);
+    if (AllocaInst *AI = canAdjustAllocaAlignment(S0->getPointerOperand(),
+                                                  EltSzInBytes, Alignment)) {
       Alignment = StackAdjustedAlignment;
-    } else {
+      if (AI->getAlignment() < Alignment)
+        AI->setAlignment(Alignment);
+    } else
       return false;
-    }
-  }
 
   BasicBlock::iterator First, Last;
   std::tie(First, Last) = getBoundaryInstrs(Chain);
   Builder.SetInsertPoint(&*Last);
 
   Value *Vec = UndefValue::get(VecTy);
 
   if (VecStoreTy) {
bool Vectorizer::vectorizeLoadChain(
   }
 
   // Update Chain to the valid vectorizable subchain.
   Chain = NewChain;
   ChainSize = Chain.size();
 
   // Load size should be 1B, 2B or multiple of 4B.
   // TODO: Should size constraint be a target hook?
-  unsigned SzInBytes = (Sz / 8) * ChainSize;
+  unsigned EltSzInBytes = Sz / 8;
+  unsigned SzInBytes = EltSzInBytes * ChainSize;
   if (SzInBytes > 2 && SzInBytes % 4 != 0) {
     DEBUG(dbgs() << "LSV: Size should be 1B, 2B "
                     "or multiple of 4B. Splitting.\n");
     if (SzInBytes == 3)
       return vectorizeLoadChain(Chain.slice(0, ChainSize - 1),
                                 InstructionsProcessed);
     auto Chains = splitOddVectorElts(Chain, Sz);
     return vectorizeLoadChain(Chains.first, InstructionsProcessed) |
bool Vectorizer::vectorizeLoadChain(
   // Check alignment restrictions.
   unsigned Alignment = getAlignment(L0);
 
   // If the load is going to be misaligned, don't vectorize it.
   if (accessIsMisaligned(SzInBytes, AS, Alignment)) {
     if (L0->getPointerAddressSpace() != 0)
       return false;
 
-    // If we're loading from an object on the stack, we control its alignment,
-    // so we can cheat and change it!
-    Value *V = GetUnderlyingObject(L0->getPointerOperand(), DL);
-    if (AllocaInst *AI = dyn_cast_or_null<AllocaInst>(V)) {
-      AI->setAlignment(StackAdjustedAlignment);
+    if (AllocaInst *AI = canAdjustAllocaAlignment(L0->getPointerOperand(),
+                                                  EltSzInBytes, Alignment)) {
       Alignment = StackAdjustedAlignment;
-    } else {
+      if (AI->getAlignment() < Alignment)
+        AI->setAlignment(Alignment);
+    } else
       return false;
-    }
-  }
 
   DEBUG({
     dbgs() << "LSV: Loads to vectorize:\n";
     for (Instruction *I : Chain)
       I->dump();
   });
 
   // getVectorizablePrefix already computed getBoundaryInstrs.  The value of
bool Vectorizer::vectorizeLoadChain(
 
   ++NumVectorInstructions;
   NumScalarsVectorized += Chain.size();
   return true;
 }
 
 bool Vectorizer::accessIsMisaligned(unsigned SzInBytes, unsigned AddressSpace,
                                     unsigned Alignment) {
-  if (Alignment % SzInBytes == 0)
+  if (Alignment >= SzInBytes)
     return false;
+
   bool Fast = false;
   bool Allows = TTI.allowsMisalignedMemoryAccesses(F.getParent()->getContext(),
                                                    SzInBytes * 8, AddressSpace,
                                                    Alignment, &Fast);
   DEBUG(dbgs() << "LSV: Target said misaligned is allowed? " << Allows
                << " and fast? " << Fast << "\n";);
   return !Allows || !Fast;
 }

test/Transforms/LoadStoreVectorizer/AMDGPU/adjust-alloca-alignment.ll

+; RUN: opt -S -load-store-vectorizer -mattr=-unaligned-buffer-access,+max-private-element-size-16 < %s | FileCheck -check-prefix=ALIGNED -check-prefix=ALL %s
+; RUN: opt -S -load-store-vectorizer -mattr=+unaligned-buffer-access,+max-private-element-size-16 < %s | FileCheck -check-prefix=UNALIGNED -check-prefix=ALL %s
+
+target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+target triple = "amdgcn--"
+
+; ALL-LABEL: @load_unknown_offset_align1_i8(
+; ALL: alloca [128 x i8], align 1
+; UNALIGNED: load <2 x i8>, <2 x i8>* %{{[0-9]+}}, align 1{{$}}
+
+; ALIGNED: load i8, i8* %ptr0, align 1{{$}}
+; ALIGNED: load i8, i8* %ptr1, align 1{{$}}
+define void @load_unknown_offset_align1_i8(i8 addrspace(1)* noalias %out, i32 %offset) #0 {
+  %alloca = alloca [128 x i8], align 1
+  %ptr0 = getelementptr inbounds [128 x i8], [128 x i8]* %alloca, i32 0, i32 %offset
+  %val0 = load i8, i8* %ptr0, align 1
+  %ptr1 = getelementptr inbounds i8, i8* %ptr0, i32 1
+  %val1 = load i8, i8* %ptr1, align 1
+  %add = add i8 %val0, %val1
+  store i8 %add, i8 addrspace(1)* %out
+  ret void
+}
+
+; ALL-LABEL: @load_unknown_offset_align1_i16(
+; ALL: alloca [128 x i16], align 1{{$}}
+; UNALIGNED: load <2 x i16>, <2 x i16>* %{{[0-9]+}}, align 1{{$}}
+
+; ALIGNED: load i16, i16* %ptr0, align 1{{$}}
+; ALIGNED: load i16, i16* %ptr1, align 1{{$}}
+define void @load_unknown_offset_align1_i16(i16 addrspace(1)* noalias %out, i32 %offset) #0 {
+  %alloca = alloca [128 x i16], align 1
+  %ptr0 = getelementptr inbounds [128 x i16], [128 x i16]* %alloca, i32 0, i32 %offset
+  %val0 = load i16, i16* %ptr0, align 1
+  %ptr1 = getelementptr inbounds i16, i16* %ptr0, i32 1
+  %val1 = load i16, i16* %ptr1, align 1
+  %add = add i16 %val0, %val1
+  store i16 %add, i16 addrspace(1)* %out
+  ret void
+}
+
+; Although the offset is unknown here, we know it is a multiple of the element size.
+; ALL-LABEL: @load_unknown_offset_align1_i32(
+; UNALIGNED: alloca [128 x i32], align 1
+; UNALIGNED: load <2 x i32>, <2 x i32>* %{{[0-9]+}}, align 1{{$}}
+
+; ALIGNED: alloca [128 x i32], align 4
+; ALIGNED: load <2 x i32>, <2 x i32>* %{{[0-9]+}}, align 4{{$}}
+define void @load_unknown_offset_align1_i32(i32 addrspace(1)* noalias %out, i32 %offset) #0 {
+  %alloca = alloca [128 x i32], align 1
+  %ptr0 = getelementptr inbounds [128 x i32], [128 x i32]* %alloca, i32 0, i32 %offset
+  %val0 = load i32, i32* %ptr0, align 1
+  %ptr1 = getelementptr inbounds i32, i32* %ptr0, i32 1
+  %val1 = load i32, i32* %ptr1, align 1
+  %add = add i32 %val0, %val1
+  store i32 %add, i32 addrspace(1)* %out
+  ret void
+}
+
+; FIXME: Should always increase alignment of the load
+; Make sure alloca alignment isn't decreased
+; ALL-LABEL: @load_alloca16_unknown_offset_align1_i32(
+; ALL: alloca [128 x i32], align 16
+
+; UNALIGNED: load <2 x i32>, <2 x i32>* %{{[0-9]+}}, align 1{{$}}
+; ALIGNED: load <2 x i32>, <2 x i32>* %{{[0-9]+}}, align 4{{$}}
+define void @load_alloca16_unknown_offset_align1_i32(i32 addrspace(1)* noalias %out, i32 %offset) #0 {
+  %alloca = alloca [128 x i32], align 16
+  %ptr0 = getelementptr inbounds [128 x i32], [128 x i32]* %alloca, i32 0, i32 %offset
+  %val0 = load i32, i32* %ptr0, align 1
+  %ptr1 = getelementptr inbounds i32, i32* %ptr0, i32 1
+  %val1 = load i32, i32* %ptr1, align 1
+  %add = add i32 %val0, %val1
+  store i32 %add, i32 addrspace(1)* %out
+  ret void
+}
+
+; ALL-LABEL: @store_unknown_offset_align1_i8(
+; ALL: alloca [128 x i8], align 1
+; UNALIGNED: store <2 x i8> <i8 9, i8 10>, <2 x i8>* %{{[0-9]+}}, align 1{{$}}
+
+; ALIGNED: store i8 9, i8* %ptr0, align 1{{$}}
+; ALIGNED: store i8 10, i8* %ptr1, align 1{{$}}
+define void @store_unknown_offset_align1_i8(i8 addrspace(1)* noalias %out, i32 %offset) #0 {
+  %alloca = alloca [128 x i8], align 1
+  %ptr0 = getelementptr inbounds [128 x i8], [128 x i8]* %alloca, i32 0, i32 %offset
+  store i8 9, i8* %ptr0, align 1
+  %ptr1 = getelementptr inbounds i8, i8* %ptr0, i32 1
+  store i8 10, i8* %ptr1, align 1
+  ret void
+}
+
+; ALL-LABEL: @store_unknown_offset_align1_i16(
+; ALL: alloca [128 x i16], align 1
+; UNALIGNED: store <2 x i16> <i16 9, i16 10>, <2 x i16>* %{{[0-9]+}}, align 1{{$}}
+
+; ALIGNED: store i16 9, i16* %ptr0, align 1{{$}}
+; ALIGNED: store i16 10, i16* %ptr1, align 1{{$}}
+define void @store_unknown_offset_align1_i16(i16 addrspace(1)* noalias %out, i32 %offset) #0 {
+  %alloca = alloca [128 x i16], align 1
+  %ptr0 = getelementptr inbounds [128 x i16], [128 x i16]* %alloca, i32 0, i32 %offset
+  store i16 9, i16* %ptr0, align 1
+  %ptr1 = getelementptr inbounds i16, i16* %ptr0, i32 1
+  store i16 10, i16* %ptr1, align 1
+  ret void
+}
+
+; Although the offset is unknown here, we know it is a multiple of the element size.
+; ALL-LABEL: @store_unknown_offset_align1_i32(
+; UNALIGNED: alloca [128 x i32], align 1
+; UNALIGNED: store <2 x i32> <i32 9, i32 10>, <2 x i32>* %{{[0-9]+}}, align 1{{$}}
+
+; ALIGNED: alloca [128 x i32], align 4
+; ALIGNED: store <2 x i32> <i32 9, i32 10>, <2 x i32>* %{{[0-9]+}}, align 4{{$}}
+define void @store_unknown_offset_align1_i32(i32 addrspace(1)* noalias %out, i32 %offset) #0 {
+  %alloca = alloca [128 x i32], align 1
+  %ptr0 = getelementptr inbounds [128 x i32], [128 x i32]* %alloca, i32 0, i32 %offset
+  store i32 9, i32* %ptr0, align 1
+  %ptr1 = getelementptr inbounds i32, i32* %ptr0, i32 1
+  store i32 10, i32* %ptr1, align 1
+  ret void
+}
+
+attributes #0 = { nounwind }
+