[PATCH] D60545: [DAGCombiner] narrow shuffle of concatenated vectors

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

LGTM. The compiler will have more opportunity to use cheaper shuffles if part of the (sub)vector is undef, so this looks like a general improvement.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17640
+
+  // shuffle (concat X, undef), (concat Y, undef), Mask -->
+  // concat (shuffle X, Y, Mask0), (shuffle X, Y, Mask1)
----------------
spatel wrote:
> sdesmalen wrote:
> > Would it be possible to move/copy this comment to the call of foldShuffleOfConcatUndefs ?
> I can do that if you think that makes the code clearer, but in general I try to make the function's doxygen comment a bit more vague in case the implementation changes/grows, but not repeat that function-level comment at the callers.
> 
> For example, I think this function could be extended to handle a pattern where the 2nd operand is not an undef value as long as the shuffle mask is not choosing elements from that 2nd operand.
> 
> Let me know if I am misunderstanding the suggestion.
When reading through DAGCombiner code I usually appreciate the "X -> Y" comments that provide an overview of transforms so you can easily see what case is optimised without having to look at further function definitions/doxygen. But that only makes sense if the function handles a single transform. So I agree that if the scope of this function widens, it makes little sense to copy this comment to the call-site.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60545/new/

https://reviews.llvm.org/D60545