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

[Eric Christopher]

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/20150622/1f002175/attachment.html>