Address two correctness issues in LoadStoreVectorizer

[asbirlea]

Summary

GetBoundryInstruction returns the last instruction as the instruction which follows or end(). Otherwise the last instruction in the boundry set is not being tested by isVectorizable().
Partially solve reordering of instructions. More extensive solution to follow.

[jlebar]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

366 ↗

(On Diff #62526)

Can we have a comment here? Maybe just

// LastInstr is not inclusive.

Or alternatively maybe it would make more sense to leave this as-is and do

// Range is [first, last).
return std::maked_pair(FirstInstr, ++LastInstr.getIterator());

?

422 ↗

(On Diff #62526)

Nit, no need for "LSV:" -- the assertion error will point to this line of code. And, can we make this message a bit more specific? Like, clearly if an assert fails, there's an error. But what is the condition we're checking here, exactly? Maybe "Range [From, To) must contain all instructions in Chain"?

test/Transforms/LoadStoreVectorizer/AMDGPU/interleaved-mayalias-store.ll

5 ↗

(On Diff #62526)

Need a comma before "as" -- it's a conjunction separating two independent clauses.

test/Transforms/LoadStoreVectorizer/x86/preserve-order32.ll

1 ↗

(On Diff #62526)

Can we have a comment somewhere indicating what we're testing? Same for the other test. And maybe we should get rid of whatever instructions aren't relevant to the thing we're checking from our CHECKs?

[arsenm]

You're missing the new lit.local.cfg for x86

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

197 ↗

(On Diff #62526)

This prints the entire block doesn't it? Maybe restrict to just the name? I think printing the entire block would be way too verbose for this, and you can already use the number to refer to the -print-before output

422 ↗

(On Diff #62526)

This message isn't descriptive of what is wrong and doesn't need the LSV

[arsenm]

test/Transforms/LoadStoreVectorizer/x86/preserve-order32.ll

1 ↗

(On Diff #62526)

Should this have -basicaa?

[asbirlea]

Addressing comments.

[jlebar]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

422 ↗

(On Diff #62526)

Need to wrap to 80 chars.

But also this message can be tightened up, I think. No need to say "Error checking if chain is vectorizable:" -- we can tell from the stacktrace printed when the assert fails.

And as written this doesn't make sense to me:

All instructions in Chain should have been to ChainInsts from [From, To).

Maybe "to" is the wrong preposition? Or do you mean "should have been added to"? I still like my original suggestion, tbh. :)

[asbirlea]

Shorten error message.

[asbirlea]

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

197 ↗

(On Diff #62546)

This was a leftover, I removed it.

423 ↗

(On Diff #62546)

I meant "added to", realized that shortly after the update. Reupdated.

test/Transforms/LoadStoreVectorizer/x86/preserve-order32.ll

2 ↗

(On Diff #62546)

I don't think so. I can added it if you think this is the case.

[asbirlea]

Adding lit.local.cfg. Renaming x86 to X86.

llvm/trunk/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

bool Vectorizer::isConsecutiveAccess(Value *A, Value *B) {
   const SCEV *OffsetSCEVA = SE.getSCEV(OpA);
   const SCEV *OffsetSCEVB = SE.getSCEV(OpB);
   const SCEV *One = SE.getConstant(APInt(BitWidth, 1));
   const SCEV *X2 = SE.getAddExpr(OffsetSCEVA, One);
   return X2 == OffsetSCEVB;
 }
 
 void Vectorizer::reorder(Instruction *I) {
+  Instruction *InsertAfter = I;
   for (User *U : I->users()) {
     Instruction *User = dyn_cast<Instruction>(U);
     if (!User || User->getOpcode() == Instruction::PHI)
       continue;
 
     if (!DT.dominates(I, User)) {
       User->removeFromParent();
-      User->insertAfter(I);
+      User->insertAfter(InsertAfter);
+      InsertAfter = User;
       reorder(User);
     }
   }
 }
 
 std::pair<BasicBlock::iterator, BasicBlock::iterator>
 Vectorizer::getBoundaryInstrs(ArrayRef<Value *> Chain) {
   Instruction *C0 = cast<Instruction>(Chain[0]);
   BasicBlock::iterator FirstInstr = C0->getIterator();
   BasicBlock::iterator LastInstr = C0->getIterator();
 
   BasicBlock *BB = C0->getParent();
   unsigned NumFound = 0;
   for (Instruction &I : *BB) {
     if (!is_contained(Chain, &I))
       continue;
 
     ++NumFound;
     if (NumFound == 1) {
       FirstInstr = I.getIterator();
-    } else if (NumFound == Chain.size()) {
+    }
+    if (NumFound == Chain.size()) {
       LastInstr = I.getIterator();
       break;
     }
   }
 
-  return std::make_pair(FirstInstr, LastInstr);
+  // Range is [first, last).
+  return std::make_pair(FirstInstr, ++LastInstr);
 }
 
 void Vectorizer::eraseInstructions(ArrayRef<Value *> Chain) {
   SmallVector<Instruction *, 16> Instrs;
   for (Value *V : Chain) {
     Value *PtrOperand = getPointerOperand(V);
     assert(PtrOperand && "Instruction must have a pointer operand.");
     Instrs.push_back(cast<Instruction>(V));
if (isa<LoadInst>(I) || isa<StoreInst>(I)) {
       else
         ChainInstrs.push_back({ &*I, Idx });
     } else if (I->mayHaveSideEffects()) {
       DEBUG(dbgs() << "LSV: Found side-effecting operation: " << *I << '\n');
       return false;
     }
   }
 
+  assert(Chain.size() == ChainInstrs.size() &&
+         "All instructions in the Chain must exist in [From, To).");
+
   for (auto EntryMem : MemoryInstrs) {
     Value *V = EntryMem.first;
     unsigned VIdx = EntryMem.second;
     for (auto EntryChain : ChainInstrs) {
       Value *VV = EntryChain.first;
       unsigned VVIdx = EntryChain.second;
       if (isa<LoadInst>(V) && isa<LoadInst>(VV))
         continue;

llvm/trunk/test/Transforms/LoadStoreVectorizer/AMDGPU/interleaved-mayalias-store.ll

 ; RUN: opt -mtriple=amdgcn-amd-amdhsa -basicaa -load-store-vectorizer -S -o - %s | FileCheck %s
 
 target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-p24:64:64-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
 
-; This is OK to vectorize the load as long as the may alias store
-; occurs before the vector load.
+; This is NOT OK to vectorize, as either load may alias either store.
 
+; CHECK: load double
 ; CHECK: store double 0.000000e+00, double addrspace(1)* %a,
-; CHECK: load <2 x double>
+; CHECK: load double
 ; CHECK: store double 0.000000e+00, double addrspace(1)* %a.idx.1
 define void @interleave(double addrspace(1)* nocapture %a, double addrspace(1)* nocapture %b, double addrspace(1)* nocapture readonly %c) #0 {
 entry:
   %a.idx.1 = getelementptr inbounds double, double addrspace(1)* %a, i64 1
   %c.idx.1 = getelementptr inbounds double, double addrspace(1)* %c, i64 1
 
   %ld.c = load double, double addrspace(1)* %c, align 8 ; may alias store to %a
   store double 0.0, double addrspace(1)* %a, align 8

llvm/trunk/test/Transforms/LoadStoreVectorizer/X86/lit.local.cfg

+if not 'X86' in config.root.targets:
+    config.unsupported = True
+

llvm/trunk/test/Transforms/LoadStoreVectorizer/X86/preserve-order32.ll

+; RUN: opt -mtriple=x86-linux -load-store-vectorizer -S -o - %s | FileCheck %s
+
+target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-p24:64:64-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+
+%struct.buffer_t = type { i32, i8* }
+
+; Check an i32 and i8* get vectorized, and that
+; the two accesses (load into buff.val and store to buff.p) preserve their order.
+
+; CHECK-LABEL: @preserve_order_32(
+; CHECK: load <2 x i32>
+; CHECK: %buff.val = load i8
+; CHECK: store i8 0
+define void @preserve_order_32(%struct.buffer_t* noalias %buff) #0 {
+entry:
+  %tmp1 = getelementptr inbounds %struct.buffer_t, %struct.buffer_t* %buff, i32 0, i32 1
+  %buff.p = load i8*, i8** %tmp1, align 8
+  %buff.val = load i8, i8* %buff.p, align 8
+  store i8 0, i8* %buff.p, align 8
+  %tmp0 = getelementptr inbounds %struct.buffer_t, %struct.buffer_t* %buff, i32 0, i32 0
+  %buff.int = load i32, i32* %tmp0, align 8
+  ret void
+}
+
+attributes #0 = { nounwind }

llvm/trunk/test/Transforms/LoadStoreVectorizer/X86/preserve-order64.ll

+; RUN: opt -mtriple=x86-linux -load-store-vectorizer -S -o - %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+
+%struct.buffer_t = type { i64, i8* }
+
+; Check an i64 and i8* get vectorized, and that
+; the two accesses (load into buff.val and store to buff.p) preserve their order.
+
+; CHECK-LABEL: @preserve_order_64(
+; CHECK: load <2 x i64>
+; CHECK: %buff.val = load i8
+; CHECK: store i8 0
+define void @preserve_order_64(%struct.buffer_t* noalias %buff) #0 {
+entry:
+  %tmp1 = getelementptr inbounds %struct.buffer_t, %struct.buffer_t* %buff, i64 0, i32 1
+  %buff.p = load i8*, i8** %tmp1, align 8
+  %buff.val = load i8, i8* %buff.p, align 8
+  store i8 0, i8* %buff.p, align 8
+  %tmp0 = getelementptr inbounds %struct.buffer_t, %struct.buffer_t* %buff, i64 0, i32 0
+  %buff.int = load i64, i64* %tmp0, align 8
+  ret void
+}
+
+attributes #0 = { nounwind }