[PATCH] [LoopVectorize]Teach Loop Vectorizer about interleaved memory access

[Adam Nemet]

In http://reviews.llvm.org/D9368#183336, @rengolin wrote:

>




> I think this review has gone a bit too far for what it is. I agree there are still many points to cover and many issues to fix, but we should do them later, after the initial implementation is in. For what it is, a prototype for strided vectorization, this patch fits the bill. It's not on by default, it's still experimental and won't be used at -O3 or anything, so we're still safe in case it does miss-compile anything.


I am very surprised by this sentiment.  I would think that if reviews provide you with bugs, you'd be happy to fix them before the code going in.  (We effectively performed the bugfixing for you.)

More importantly though, you're incorrect with your facts.  Existing paths in LAA were also modified by this patch which affect things like the LoopVectorizer which is on by default.

You also seem to suggest that the changes requested were mostly minor or cosmetic.  Clearly the placement of a rather expensive analysis is not a cosmetic change but a basic design decision that should be debated pre-commit.

There is extensive work happening on LAA, so wrong starts like this can be pretty disruptive for other people working on the same module.

Also as I am sure Hao can attest, DependenceAnalysis is a pretty complex topic so it takes time to actually think through the analysis and it's better to do this while we're all focused on the problem at hand.  We caught multiple issues in this category and there were probably only one or two cosmetic comments.

I agree that the review lasted longer than usual but I don't see the main reason being our "drastic code reviews".

It would have been much easier if the patch was split up as I originally requested.  The existing modularity allowed for this but the patch didn't take advantage of it.  Also as we were finding correctness/design issues with the patch, Hao started improving the code (in actually very nice ways, thanks!!) which then needed new reviews again, etc.

We're also in different time zones which didn't help either but I feel like the community should be able to put up with this much slack.

> Adam, if you're happy that Hao's last changes address your concerns, please approve the patch and let's start round 2 on a new review.


Yeah, the LAA parts look good now.


http://reviews.llvm.org/D9368

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/