This is an archive of the discontinued LLVM Phabricator instance.

Fixed a failure in cost calculation for vector GEP
Needs ReviewPublic

Authored by delena on Nov 25 2015, 2:06 AM.

Details

Reviewers
gilr
Summary

Cost calculation for vector GEP failed with due to invalid cast to GEP index operand.
The bug is fixed, added a test.

Diff Detail

Repository
rL LLVM

Event Timeline

delena updated this revision to Diff 41116.Nov 25 2015, 2:06 AM
delena retitled this revision from to Fixed a failure in cost calculation for vector GEP.
delena updated this object.
delena added reviewers: gilr, jingyue.
delena set the repository for this revision to rL LLVM.
delena added subscribers: llvm-commits, hfinkel.
jingyue added inline comments.Nov 25 2015, 9:49 AM
../include/llvm/Analysis/TargetTransformInfoImpl.h
421

I don't understand why you use getSplatValue here. getSplatValue returns the element type only when all the elements in the array/vector are the same. Are you supposed to also protect the cases where the elements are different?

../lib/Analysis/VectorUtils.cpp
419–420

This if-check seems unnecessary. The one at Line 424 already checks for ConstantDataVector.

delena added inline comments.Nov 25 2015, 11:44 PM
../include/llvm/Analysis/TargetTransformInfoImpl.h
421

It may be a vector of constants. In this case the ConstIdx = 0.
For structs, the index is always splat or a scalar constant.

../lib/Analysis/VectorUtils.cpp
419–420

Constant and ConstantDataVector are different types. In case of all-zero-vector it is "Constant".

jingyue requested changes to this revision.Nov 29 2015, 12:14 PM
jingyue edited edge metadata.

LGTM with some minors

../include/llvm/Analysis/TargetTransformInfoImpl.h
421

It makes sense now. Please add a comment explaining why we need to use getSplatValue. Thanks!

../lib/Analysis/VectorUtils.cpp
419–420

http://llvm.org/docs/doxygen/html/Constants_8cpp_source.html#l01435

ConstantDataVector is a subclass of Constant. Constant::getSplatValue checks if the constant is a ConstantDataVector and if so invokes ConstantDataVector::getSplatValue. So I think it's no longer necessary to check for ConstantDataVector in Line 421-422.

This revision now requires changes to proceed.Nov 29 2015, 12:14 PM
delena updated this revision to Diff 41371.Nov 30 2015, 12:52 AM
delena edited edge metadata.
delena marked 2 inline comments as done.

Jingyue, thank you for the review. I addressed all comments and updated the patch.

jingyue resigned from this revision.Feb 5 2016, 5:48 PM
jingyue removed a reviewer: jingyue.

Committed in r254408.