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

797

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

[arsenm]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

797

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

797

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

798

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.)

1028–1029

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

798

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

798

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

 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Utils/Local.h"
 #include "llvm/Transforms/Vectorize.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "load-store-vectorizer"
 STATISTIC(NumVectorInstructions, "Number of vector accesses generated");
 STATISTIC(NumScalarsVectorized, "Number of scalar accesses vectorized");
 
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);
-      Alignment = StackAdjustedAlignment;
-    } else {
+    unsigned NewAlign = getOrEnforceKnownAlignment(S0->getPointerOperand(),
+                                                   StackAdjustedAlignment,
+                                                   DL, S0, nullptr, &DT);
+    if (NewAlign < StackAdjustedAlignment)
       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);
-      Alignment = StackAdjustedAlignment;
-    } else {
+    unsigned NewAlign = getOrEnforceKnownAlignment(L0->getPointerOperand(),
+                                                   StackAdjustedAlignment,
+                                                   DL, L0, nullptr, &DT);
+    if (NewAlign < StackAdjustedAlignment)
       return false;
-    }
+
+    Alignment = NewAlign;
   }
 
   DEBUG({
     dbgs() << "LSV: Loads to vectorize:\n";
     for (Instruction *I : Chain)
       I->dump();
   });
 
bool Vectorizer::vectorizeLoadChain(
   ++NumVectorInstructions;
   NumScalarsVectorized += Chain.size();
   return true;
 }
 
 bool Vectorizer::accessIsMisaligned(unsigned SzInBytes, unsigned AddressSpace,
                                     unsigned Alignment) {
   if (Alignment % SzInBytes == 0)
     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
+}
+
+; FIXME: Although the offset is unknown here, we know it is a multiple
+; of the element size, so should still be align 4
+
+; ALL-LABEL: @load_unknown_offset_align1_i32(
+; ALL: alloca [128 x i32], align 1
+; UNALIGNED: load <2 x i32>, <2 x i32>* %{{[0-9]+}}, align 1{{$}}
+
+; ALIGNED: load i32, i32* %ptr0, align 1
+; ALIGNED: load i32, i32* %ptr1, align 1
+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
+}
+
+; FIXME: Although the offset is unknown here, we know it is a multiple
+; of the element size, so it still should be align 4.
+
+; ALL-LABEL: @store_unknown_offset_align1_i32(
+; ALL: alloca [128 x i32], align 1
+
+; UNALIGNED: store <2 x i32> <i32 9, i32 10>, <2 x i32>* %{{[0-9]+}}, align 1{{$}}
+
+; ALIGNED: store i32 9, i32* %ptr0, align 1
+; ALIGNED: store i32 10, i32* %ptr1, align 1
+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 }
+