This is an archive of the discontinued LLVM Phabricator instance.

Make SLPVectorizer pass the right type argument to getCmpSelInstrCost()
ClosedPublic

Authored by jonpa on Apr 3 2017, 8:07 AM.

Download Raw Diff

Details

Reviewers

nadav
mssimpso

Summary

(This was also on llvm-commits last week without test case: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170327/442107.html)

I found that the SLP vectorizer does not call getCmpSelInstrCost() for a
compare with the proper type. It currently passes i1 or a vector of i1s,
instead of the type of the compare operand, like LoopVectorizer does.

My patch fixes this by getting the right scalar type for compares. No
regressions after applying this patch.

The test case is for SystemZ, and depends on https://reviews.llvm.org/D29631#09b6b393.

Diff Detail

Event Timeline

jonpa created this revision.Apr 3 2017, 8:07 AM

Herald added a subscriber: mzolotukhin. · View Herald TranscriptApr 3 2017, 8:07 AM

Hi Jonas,

This makes sense to me - just a couple of minor comments.

lib/Transforms/Vectorize/SLPVectorizer.cpp
1705	Why not use an "else if" here?
test/Transforms/SLPVectorizer/SystemZ/SLP-cmp-cost-query.ll
2	You'll need a "REQUIRES: asserts" line to check debug output.

changed to "if else".
Test updated with '; REQUIRES: asserts'

jonpa marked 2 inline comments as done.Apr 6 2017, 11:53 PM

LGTM.

This revision is now accepted and ready to land.Apr 7 2017, 10:22 AM

Thanks for review!
r300061

Revision Contents

Path

Size

lib/

Transforms/

Vectorize/

SLPVectorizer.cpp

2 lines

test/

Transforms/

SLPVectorizer/

SystemZ/

SLP-cmp-cost-query.ll

36 lines

Diff 94488

lib/Transforms/Vectorize/SLPVectorizer.cpp

	Show First 20 Lines • Show All 1,696 Lines • ▼ Show 20 Lines
	}			}

	int BoUpSLP::getEntryCost(TreeEntry *E) {			int BoUpSLP::getEntryCost(TreeEntry *E) {
	ArrayRef<Value*> VL = E->Scalars;			ArrayRef<Value*> VL = E->Scalars;

	Type *ScalarTy = VL[0]->getType();			Type *ScalarTy = VL[0]->getType();
	if (StoreInst *SI = dyn_cast<StoreInst>(VL[0]))			if (StoreInst *SI = dyn_cast<StoreInst>(VL[0]))
	ScalarTy = SI->getValueOperand()->getType();			ScalarTy = SI->getValueOperand()->getType();
				else if (CmpInst *CI = dyn_cast<CmpInst>(VL[0]))
				mssimpsoUnsubmitted Done Reply Inline Actions Why not use an "else if" here? mssimpso: Why not use an "else if" here?
				ScalarTy = CI->getOperand(0)->getType();
	VectorType *VecTy = VectorType::get(ScalarTy, VL.size());			VectorType *VecTy = VectorType::get(ScalarTy, VL.size());

	// If we have computed a smaller type for the expression, update VecTy so			// If we have computed a smaller type for the expression, update VecTy so
	// that the costs will be accurate.			// that the costs will be accurate.
	if (MinBWs.count(VL[0]))			if (MinBWs.count(VL[0]))
	VecTy = VectorType::get(			VecTy = VectorType::get(
	IntegerType::get(F->getContext(), MinBWs[VL[0]].first), VL.size());			IntegerType::get(F->getContext(), MinBWs[VL[0]].first), VL.size());

	▲ Show 20 Lines • Show All 3,446 Lines • Show Last 20 Lines

test/Transforms/SLPVectorizer/SystemZ/SLP-cmp-cost-query.ll

This file was added.

				; REQUIRES: asserts
				; RUN: opt -mtriple=systemz-unknown -mcpu=z13 -slp-vectorizer -debug-only=SLP \
				mssimpsoUnsubmitted Done Reply Inline Actions You'll need a "REQUIRES: asserts" line to check debug output. mssimpso: You'll need a "REQUIRES: asserts" line to check debug output.
				; RUN: -S -disable-output < %s \|& FileCheck %s
				;
				; Check that SLP vectorizer gets the right cost difference for a compare
				; node.

				; Function Attrs: norecurse nounwind readonly
				define void @fun(i8* nocapture, i32 zeroext) local_unnamed_addr #0 {
				.lr.ph.preheader:
				br label %.lr.ph

				.lr.ph: ; preds = %.lr.ph.preheader, %.lr.ph
				%2 = phi i32 [ %., %.lr.ph ], [ undef, %.lr.ph.preheader ]
				%3 = phi i32 [ %.9, %.lr.ph ], [ undef, %.lr.ph.preheader ]
				%4 = icmp ult i32 %2, %1
				%5 = select i1 %4, i32 0, i32 %1
				%. = sub i32 %2, %5
				%6 = icmp ult i32 %3, %1
				%7 = select i1 %6, i32 0, i32 %1
				%.9 = sub i32 %3, %7
				%8 = zext i32 %. to i64
				%9 = getelementptr inbounds i8, i8* %0, i64 %8
				%10 = load i8, i8* %9, align 1
				%11 = zext i32 %.9 to i64
				%12 = getelementptr inbounds i8, i8* %0, i64 %11
				%13 = load i8, i8* %12, align 1
				%14 = icmp eq i8 %10, %13
				br i1 %14, label %.lr.ph, label %._crit_edge

				._crit_edge: ; preds = %.lr.ph
				ret void

				; CHECK: SLP: Adding cost -1 for bundle that starts with %4 = icmp ult i32 %2, %1.
				}