[PATCH] D25580: [PowerPC] Improve handling of BUILD_VECTOR nodes (integer results)

[Andrea Di Biagio via llvm-commits]

andreadb added a comment.

Hi Nemanja,

I agree with Eric when he says that this patch does too many things.

In https://reviews.llvm.org/D25580#576344, @nemanjai wrote:

> In https://reviews.llvm.org/D25580#576141, @echristo wrote:
>
> > Wow. This patch is huge. I don't know if some/any of it can be split up offhand.
> >
> > Some inline comments.
> >
> > -eric
>
>
> I was struggling to decide how to split this up because I realize the patch is kind of hefty. But all of it is improvements to the handling of BUILD_VECTOR nodes and most parts of the patch are dependent on other chunks. I think that the two new functions (combining a build vector of conversions to a conversion of build vector and combining consecutive loads) can be split into separate patches. However, I'm not sure if that accomplishes much since that code is already sitting in separate functions.


I agree with Eric here. It looks to me that this patch really does too many things. Hopefully it shouldn't be too complicated to split this patch into smaller (but easier to review) patches.
The other thing I have noticed is that no extra tests have been added by your patch. Given the amount of extra logic added by the patch, I have to admit that I was a bit surprised not to see any extra test coverage. Is it because the coverage for those features is already in the regression suite?


Repository:
  rL LLVM

https://reviews.llvm.org/D25580