[PATCH] D18237: [SLPVectorizer] Change MinVecRegSize from 128 bits to 64 bits
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 17 12:26:06 PDT 2016
mzolotukhin added a comment.
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.
More information about the llvm-commits