[PATCH] D18237: [SLPVectorizer] Change MinVecRegSize from 128 bits to 64 bits

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Jongwon,

Thanks for working on this, I think that's a natural improvement over what we have now. I agree with Chad's remarks and have some more comments.

The patch now includes two semantical parts:

- Change default `MinVecRegSize` value for AArch64.
- Actually use `MinVecRegSize` in `tryToVectorizeList` and friends.

They look to me completely separated (and, in the end, needed), so I'd suggest tackling them in separate patches.

For the first part - changing the default value - I totally agree with Chad that it should be done via a target hook. You could take a look at how `MaxVecRegSize` is implemented. In general, `SLPVectorizer.cpp` shouldn't contain any target-specific code, and all target-dependent info should be requested via hooks (i.e. having a variable `IsAArch64` might be a bad idea).
Also, for this kind of change it would be nice to show the impact on compilation and execution time. LLVM test-suite is good for this, results on  other popular testsuites (e.g. SPEC) are also valueable.

The second part is good in general, one possible improvement you could do (if you have time for this) is to refactor `vectorizeStoreChain` and `tryToVectorizeList` functions. They'll be very similar when `tryToVectorizeList` is parametrized with a size too. That potentially should remove a lot of duplication in the code, which is always good.

Also, you'll need new tests for the second part, currently I assume the test you have is for the first part.

Thanks,
Michael


http://reviews.llvm.org/D18237