[PATCH] D42896: [SelectionDAG] Add initial implementation of TargetLowering::SimplifyDemandedVectorElts

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon marked 2 inline comments as done.
RKSimon added a comment.

Finally gotten back to a non-MSVC build machine, I'll get those signed/unsigned warning fixed up properly in the next diff.



================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:1317
+      KnownUndef.setAllBits();
+      return TLO.CombineTo(Op, TLO.DAG.getUNDEF(VT));
+    }
----------------
zvi wrote:
> Is this case is covered by tests?
IIRC yes - I think it gets created in the sse3.ll test that changes.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:1393
+  }
+  case ISD::VECTOR_SHUFFLE: {
+    ArrayRef<int> ShuffleMask = cast<ShuffleVectorSDNode>(Op)->getMask();
----------------
zvi wrote:
> Just wondering how an existing VECTOR_SHUFFLE DAGCombiner simplification, such as shuffle(splat) -> splat, relates to this analysis.
> Would it be more powerful to move it here? Should the existing DAGCombiner simplification use this analysis or neither because this analysis+simplification will indirectly make the DAGCombiner simplification more powerful?
Yes, a number of those combine could possibly be moved. I've avoided doing too much that extends beyond existing features as they can have a large effect on x86 shuffle tests, for instance simplifying shuffle masks in here causes a number of regressions that I'm still triaging.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:28192
 
+bool X86TargetLowering::SimplifyDemandedVectorEltsForTargetNode(
+    SDValue Op, const APInt &DemandedElts, APInt &KnownUndef, APInt &KnownZero,
----------------
zvi wrote:
> Sorry if i'm missing here something obvious, but is this overload needed?
It was mainly added as part of tests while developing this patch - as I mentioned in the summary I'm happy to remove it from this patch, but I intend to add it in a future patch quite quickly.


Repository:
  rL LLVM

https://reviews.llvm.org/D42896