[llvm] r238918 - AVX-512: VSHUFPD instruction selection - code improvements

[Chandler Carruth]

Thanks for following-up Eric. Based on that and the silence from others to
point in a different direction, I'll move forward.

Sorry for the delay Elena, but I wanted to give folks as much time to
comment as I could.

On Mon, Jun 22, 2015 at 6:57 AM Eric Christopher <echristo at google.com>
wrote:

> Hi Chandler,
>
> I think that backing this code out and asking for a design review of the
> shuffle code makes sense. The work done doesn't necessarily seem like a
> straight extension of the existing code and could use some additional
> testing like the last set of shuffle work you did. I'm a big fan of the
> yardage automation script you wrote to keep things consistent and any new
> shuffle code should use that if possible.
>
> Thanks
>
> -eric
>
>
>  On Wed, Jun 17, 2015, 1:29 AM Chandler Carruth <chandlerc at google.com>
> wrote:
>
> Elena, you seem to have missed where I specifically said that these kinds
> of refactorings should not go in without precommit review since it is not
> at all clear what direction this code is actually going, and since you
> don't understand the testing infrastructure that was built up for vector
> shuffle lowering.
>
> I'll try to be more explicit: do not commit without approval, and do not
> approve patches for commit regarding x86 vector shuffle lowering.
>
> I know this is a bit harsh, but here is my problem: you have not addressed
> my comments or my concerns, you aren't following the latest best practices
> for testing them and don't seem to understand those testing practices, and
> you seem to be building the avx512 shuffle lowering in a way I disagree
> with design-wise and committing it without review. I don't know how to
> realistically correct this without building up more bad code in the x86
> backend that will have to be replaced later other than to insist on
> precommit review. I'm CC-ing some other folks to try and make sure I'm not
> misunderstanding what is going on here, and to double-check that this is
> the correct response.
>
>  Assuming that folks generally agree, and because we don't seem to be
> making progress with post-commit review, I suggest the following course of
> action:
>
> 1) Revert the functional changes to the vector shuffle lowering code in
> X86ISelLowering.cpp until the design has been discussed.
>
> 2) Start a discussion on llvmdev for how to design the avx512 shuffle
> lowering based on the new general shuffle lowering infrastructure. This
> follows exactly the process that *I* used to start working on vector
> shuffle lowering.
>
> 3) Based on the discussion in #2, it should be clear what the correct
> initial patch will look like. Post that for precommit review, preferably
> (since I'm likely one of a couple of good reviewers) using Phabricator.
>
> Some more comments in line on specifics:
>
> On Thu, Jun 4, 2015 at 1:03 AM Demikhovsky, Elena <
> elena.demikhovsky at intel.com> wrote:
>
> Chandler,
>
>
>
> I re-generated the previously deleted test file
> test/CodeGen/X86/vector-shuffle-512-v8.ll and put it back into repository.
>
>
> I don't understand why you are continuing to use the other tests though?
>
>
>
> As far I understand, the vector-shuffle-512-v8.ll does not check
> correctness of the shuffles, it just checks that noting fails.
>
>
> That is pretty clearly not the case. The test is checking for specific
> instruction sequences on each pattern.
>
>
>
> And it should be re-generated after each optimization, right?
>
>
> Yes, and most specifically the diff should show exactly what the change
> entailed.
>
>
>
> *From:* Demikhovsky, Elena
> *Sent:* Thursday, June 04, 2015 09:02
>
> Hi Chandler,
>
>
>
> First of all, I did not add any functional changes to AVX2. So the AVX2
> code state did not change at all.
>
>
> You made the code significantly more complex and hard to understand.
> Perhaps this complexity is worth while to share code with AVX-512, but that
> isn't clear to me because we don't actually have a design for how AVX-512
> shuffle lowering should work yet.
>
> SHUFPD is common logic for AVX2 and AVX-512 and I put it in a separate
> function, like you did with SHUFPS.
>
>
> These are not necessarily equivalent. They may be, they may not be. SHUFPD
> has common logic between SSE2 and AVX2 as well and yet the code was more
> clear without trying to overtly share it.
>
>
>
> I worked on AVX-512 shuffles, 32 and 64-bit elements. Now the functions
> lowerV8X64VectorShuffle() and lowerV16X32VectorShuffle() are short and
> clear.
>
> Each AVX-512 shuffle is replaced now with 1 instruction - instead of 5-6
> that were before. This is the main benefit of AVX-512 - we don’t need more
> than one instruction.
>
> The new ISA provides many instructions that shuffle 512-bit vectors. In
> the worst case I put an instruction with variable permutations, which loads
> indices from memory.
>
> In order to avoid the load, I’m trying to match PSHUFD, SHUFPS, VPERMIL,
> VALIGN patterns. I added a lit test for each pattern.
>
> The work on V8X64 and V16X32 is almost completed. We also have 128-bit
> shuffles that will be implemented soon and may give some code improvements.
>
>  I'm sorry that I didn't review each of these patches as they went in,
> but they didn't go for precommit review, and this is where I started diving
> into the code.
>
> I think that the overall design needs a discussion with the community, and
> some agreement that it is the correct design first. Your patches, the
> amount of comments, and the complete *deletion* of tests and failure to add
> new tests that follow the existing pattern for all other vector shuffle
> testing makes it very hard for me to conclude that these patches are in any
> way "obvious" and reasonable to commit without review.
>
> I’m going to look at your utility. ( It is still not clear for me why do
> we need it. )
>
>
> Note that there was code review for the utility and discussion in the
> community about it. If you disagree with the usage of it or the design of
> the vector shuffle tests, then raise that as a separate point of discussion.
>
> What I find really unacceptable is to simply implement AVX-512 in a
> different manner with a different approach to testing than the rest of the
> vector shuffle lowering with no discussion with the other significant
> contributors to the x86 backend, and that is what has happened here.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150623/87ceada7/attachment.html>