[llvm] r180875 - SROA: Generate selects instead of shuffles when blending values because this is the cannonical form.

[Nadav Rotem]

> 
> Why?

Hello,

> (not that i disagree, I just don't think you've explained it fully...)

I think that the motivation for this patch is clear in light of the discussion that we had on the mailing list yesterday.  The compiler is very conservative about optimizing shuffles and it usually does not modify shuffles. However we do optimize vector selects.  Selects are simpler, to understand and optimize and they do not require 1000 lines of c++ code to lower.  

> Also, why doesn't instcombine canonicalize shuffles to selects if they are the canonical form?

This is also something that needs to be done, but it is not a part of this patch. 

> The whole way I picked a form was to look at what would get canonicalized if the user wrote it, and match that. Until the rest of the pipeline is canonicalizing in this way, I worry greatly about this being a deeply fragile hack.

I don't see why this is a hack.  The current design is that we don't touch or generate new shuffles. Modifying to SROA generate selects (that are equivalent to the shuffles that were previously generated) only continues with the current design.  Are you suggesting that we start optimizing shuffles at the IR level ?

> Also, as select has *numerous* other problems, it would seem really important to teach the backend to optimize shuffles that are blends rather than paper over this deficiency in SROA.

The backends already do a good job lowering shuffle-blends. The problem is with other optimizations, such as SimplifyDemandedBits, that do not touch shuffles, but can handle selects. 

Why do you say that this is a deficiency in SROA ? 

> Why not call this a select? that's what it is.

You are blending two values to create a single vector. I think that the name blend reflects the operation, and not the actual instruction that we generate. Maybe it will be a good idea to change the name 'shuffle' to 'widen' because we widen the data inside the vector.  

> 
> Here and below you have significantly decreased the specificity of the check. Can you please preserve that? I really dislike checking instructions occur but not checking that the values they produce are
> used in the correct context. While sometimes it is impossible, here the code *already did that*. =/

The test has four lines, and we check every single instruction using CHECK-NEXT.  I don't think that matching the variable name adds and safety to the test. 

Thanks,
Nadav



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130501/348c952e/attachment.html>