[PATCH] D35788: [DAGCombiner] Extending pattern detection for vector shuffle.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few minors but it looks like its almost there



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14090
 
+  if (!(VecIn1.getNode() && VecIn2.getNode() &&
+        (VecIn1.getOpcode() == ISD::EXTRACT_SUBVECTOR) &&
----------------
SDValue has a bool operator for this - you should be able to just test with:
```
if (!(VecIn1 && VecIn2 &&
```


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14269
+  // vector access index and adjust the VectorMask and
+  // VecIn accordingly.
+  if (VecIn.size() == 2 && NumElems == ValidElems) {
----------------
Add a TODO to support UNDEF and zero entries?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14284
+    if ((NearestPow2 = PowerOf2Ceil(MaxIndex)) &&
+        ((NumElems * 2) < NearestPow2)) {
+      unsigned SplitSize = NearestPow2 / 2;
----------------
Don't calculate NearestPow2 inside the if, pull it out and just test for non-zero NearestPow2 in the if:
```
NearestPow2 = PowerOf2Ceil(MaxIndex);
if (NearestPow2 && ((NumElems * 2) < NearestPow2)) {
```


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14286
+      unsigned SplitSize = NearestPow2 / 2;
+      if (0 == SplitSize % 2) {
+        EVT SplitVT = EVT::getVectorVT(*DAG.getContext(),
----------------
Isn't this just SplitSize > 1 ?


https://reviews.llvm.org/D35788