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

[Nemanja Ivanovic via llvm-commits]

nemanjai added a comment.

In https://reviews.llvm.org/D25580#576421, @andreadb wrote:

> 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?


I will certainly re-work this to try to split it up into a few patches. However, I'm not sure why you say there is no new test coverage. The test case "build-vector-tests.ll" is part of the patch and it is a completely new test case. I have a feeling that Phabricator has collapsed it in your view and you've just missed it.


Repository:
  rL LLVM

https://reviews.llvm.org/D25580