<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 11, 2015 at 6:30 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: chandlerc<br>
Date: Wed Feb 11 20:30:56 2015<br>
New Revision: 228899<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=228899&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=228899&view=rev</a><br>
Log:<br>
[slp] Fix a nasty bug in the SLP vectorizer that Joerg pointed out.<br></blockquote><div><br></div><div>FYI, Joerg indicated this is at least in some cases a regression from 3.5, so it seems good to pull into the 3.6 branch Hans.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Apparently some code finally started to tickle this after my<br>
canonicalization changes to instcombine.<br>
<br>
The bug stems from trying to form a vector type out of scalars that<br>
aren't compatible at all. In this example, from x86_mmx values. The code<br>
in the vectorizer that checks for reasonable types whas checking for<br>
aggregates or vectors, but there are lots of other types that should<br>
just never reach the vectorizer.<br>
<br>
Debugging this was made more confusing by the lie in an assert in<br>
VectorType::get() -- it isn't that the types are *primitive*. The types<br>
must be integer, pointer, or floating point types. No other types are<br>
allowed.<br>
<br>
I've improved the assert and added a helper to the vectorizer to handle<br>
the element type validity checks. It now re-uses the VectorType static<br>
function and then further excludes weird target-specific types that we<br>
probably shouldn't be touching here (x86_fp80 and ppc_fp128). Neither of<br>
these are really reachable anyways (neither 80-bit nor 128-bit things<br>
will get vectorized) but it seems better to just eagerly exclude such<br>
nonesense.<br>
<br>
I've added a test case, but while it definitely covers two of the paths<br>
through this code there may be more paths that would benefit from test<br>
coverage. I'm not familiar enough with the SLP vectorizer to synthesize<br>
test cases for all of these, but was able to update the code itself by<br>
inspection.<br>
<br>
Added:<br>
llvm/trunk/test/Transforms/SLPVectorizer/X86/bad_types.ll<br>
Modified:<br>
llvm/trunk/lib/IR/Type.cpp<br>
llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>
<br>
Modified: llvm/trunk/lib/IR/Type.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Type.cpp?rev=228899&r1=228898&r2=228899&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Type.cpp?rev=228899&r1=228898&r2=228899&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/IR/Type.cpp (original)<br>
+++ llvm/trunk/lib/IR/Type.cpp Wed Feb 11 20:30:56 2015<br>
@@ -708,9 +708,10 @@ VectorType::VectorType(Type *ElType, uns<br>
VectorType *VectorType::get(Type *elementType, unsigned NumElements) {<br>
Type *ElementType = const_cast<Type*>(elementType);<br>
assert(NumElements > 0 && "#Elements of a VectorType must be greater than 0");<br>
- assert(isValidElementType(ElementType) &&<br>
- "Elements of a VectorType must be a primitive type");<br>
-<br>
+ assert(isValidElementType(ElementType) && "Element type of a VectorType must "<br>
+ "be an integer, floating point, or "<br>
+ "pointer type.");<br>
+<br>
LLVMContextImpl *pImpl = ElementType->getContext().pImpl;<br>
VectorType *&Entry = ElementType->getContext().pImpl<br>
->VectorTypes[std::make_pair(ElementType, NumElements)];<br>
<br>
Modified: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=228899&r1=228898&r2=228899&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=228899&r1=228898&r2=228899&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp Wed Feb 11 20:30:56 2015<br>
@@ -84,6 +84,18 @@ static const unsigned AliasedCheckLimit<br>
// This limit is useful for very large basic blocks.<br>
static const unsigned MaxMemDepDistance = 160;<br>
<br>
+/// \brief Predicate for the element types that the SLP vectorizer supports.<br>
+///<br>
+/// The most important thing to filter here are types which are invalid in LLVM<br>
+/// vectors. We also filter target specific types which have absolutely no<br>
+/// meaningful vectorization path such as x86_fp80 and ppc_f128. This just<br>
+/// avoids spending time checking the cost model and realizing that they will<br>
+/// be inevitably scalarized.<br>
+static bool isValidElementType(Type *Ty) {<br>
+ return VectorType::isValidElementType(Ty) && !Ty->isX86_FP80Ty() &&<br>
+ !Ty->isPPC_FP128Ty();<br>
+}<br>
+<br>
/// \returns the parent basic block if all of the instructions in \p VL<br>
/// are in the same block or null otherwise.<br>
static BasicBlock *getSameBlock(ArrayRef<Value *> VL) {<br>
@@ -1148,7 +1160,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Val<br>
Type *SrcTy = VL0->getOperand(0)->getType();<br>
for (unsigned i = 0; i < VL.size(); ++i) {<br>
Type *Ty = cast<Instruction>(VL[i])->getOperand(0)->getType();<br>
- if (Ty != SrcTy || Ty->isAggregateType() || Ty->isVectorTy()) {<br>
+ if (Ty != SrcTy || !isValidElementType(Ty)) {<br>
BS.cancelScheduling(VL);<br>
newTreeEntry(VL, false);<br>
DEBUG(dbgs() << "SLP: Gathering casts with different src types.\n");<br>
@@ -3294,7 +3306,7 @@ unsigned SLPVectorizer::collectStores(Ba<br>
<br>
// Check that the pointer points to scalars.<br>
Type *Ty = SI->getValueOperand()->getType();<br>
- if (Ty->isAggregateType() || Ty->isVectorTy())<br>
+ if (!isValidElementType(Ty))<br>
continue;<br>
<br>
// Find the base pointer.<br>
@@ -3335,7 +3347,7 @@ bool SLPVectorizer::tryToVectorizeList(A<br>
<br>
for (int i = 0, e = VL.size(); i < e; ++i) {<br>
Type *Ty = VL[i]->getType();<br>
- if (Ty->isAggregateType() || Ty->isVectorTy())<br>
+ if (!isValidElementType(Ty))<br>
return false;<br>
Instruction *Inst = dyn_cast<Instruction>(VL[i]);<br>
if (!Inst || Inst->getOpcode() != Opcode0)<br>
@@ -3555,7 +3567,7 @@ public:<br>
return false;<br>
<br>
Type *Ty = B->getType();<br>
- if (Ty->isVectorTy())<br>
+ if (!isValidElementType(Ty))<br>
return false;<br>
<br>
ReductionOpcode = B->getOpcode();<br>
<br>
Added: llvm/trunk/test/Transforms/SLPVectorizer/X86/bad_types.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SLPVectorizer/X86/bad_types.ll?rev=228899&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SLPVectorizer/X86/bad_types.ll?rev=228899&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/SLPVectorizer/X86/bad_types.ll (added)<br>
+++ llvm/trunk/test/Transforms/SLPVectorizer/X86/bad_types.ll Wed Feb 11 20:30:56 2015<br>
@@ -0,0 +1,50 @@<br>
+; RUN: opt < %s -basicaa -slp-vectorizer -S -mcpu=corei7-avx | FileCheck %s<br>
+<br>
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"<br>
+target triple = "x86_64-unknown-linux-gnu"<br>
+<br>
+define void @test1(x86_mmx %a, x86_mmx %b, i64* %ptr) {<br>
+; Ensure we can handle x86_mmx values which are primitive and can be bitcast<br>
+; with integer types but can't be put into a vector.<br>
+;<br>
+; CHECK-LABEL: @test1<br>
+; CHECK: store i64<br>
+; CHECK: store i64<br>
+; CHECK: ret void<br>
+entry:<br>
+ %a.cast = bitcast x86_mmx %a to i64<br>
+ %b.cast = bitcast x86_mmx %b to i64<br>
+ %a.and = and i64 %a.cast, 42<br>
+ %b.and = and i64 %b.cast, 42<br>
+ %gep = getelementptr i64* %ptr, i32 1<br>
+ store i64 %a.and, i64* %ptr<br>
+ store i64 %b.and, i64* %gep<br>
+ ret void<br>
+}<br>
+<br>
+define void @test2(x86_mmx %a, x86_mmx %b) {<br>
+; Same as @test1 but using phi-input vectorization instead of store<br>
+; vectorization.<br>
+;<br>
+; CHECK-LABEL: @test2<br>
+; CHECK: and i64<br>
+; CHECK: and i64<br>
+; CHECK: ret void<br>
+entry:<br>
+ br i1 undef, label %if.then, label %exit<br>
+<br>
+if.then:<br>
+ %a.cast = bitcast x86_mmx %a to i64<br>
+ %b.cast = bitcast x86_mmx %b to i64<br>
+ %a.and = and i64 %a.cast, 42<br>
+ %b.and = and i64 %b.cast, 42<br>
+ br label %exit<br>
+<br>
+exit:<br>
+ %a.phi = phi i64 [ 0, %entry ], [ %a.and, %if.then ]<br>
+ %b.phi = phi i64 [ 0, %entry ], [ %b.and, %if.then ]<br>
+ tail call void @f(i64 %a.phi, i64 %b.phi)<br>
+ ret void<br>
+}<br>
+<br>
+declare void @f(i64, i64)<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>