[PATCH] D16217: [X86][SSE] Add zero element and general 64-bit VZEXT_LOAD support to EltsFromConsecutiveLoads

[Sanjay Patel via llvm-commits]

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

LGTM, but see inline comments for some nits.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:5438
@@ -5441,2 +5437,3 @@
                                         SDLoc &DL, SelectionDAG &DAG,
+                                        const X86Subtarget *Subtarget,
                                         bool isAfterLegalize) {
----------------
I don't see any use of the subtarget param. Remove here and and no need to change the callers?

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:5481
@@ -5479,1 +5480,3 @@
 
+  // Should we return this as a BUILD_VECTOR instead?
+  if ((ZeroMask | UndefMask).count() == NumElems)
----------------
Please mark this comment as a 'TODO' or 'FIXME' for easier grepping.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:5497-5498
@@ +5496,4 @@
+      LoadSDNode *LD = cast<LoadSDNode>(Elt);
+      if (!DAG.isConsecutiveLoad(LD, LDBase, Elt.getValueSizeInBits() / 8,
+                                 i - FirstLoadedElt)) {
+        IsConsecutiveLoad = false;
----------------
I think the preferred method is to use 'VT.getStoreSize()' in these situations.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:5543-5544
@@ -5513,6 +5542,4 @@
 
-  //TODO: The code below fires only for for loading the low v2i32 / v2f32
-  //of a v4i32 / v4f32. It's probably worth generalizing.
-  EVT EltVT = VT.getVectorElementType();
-  if (NumElems == 4 && LastLoadedElt == 1 && (EltVT.getSizeInBits() == 32) &&
-      DAG.getTargetLoweringInfo().isTypeLegal(MVT::v2i64)) {
+  int LoadSize =
+      (1 + LastLoadedElt - FirstLoadedElt) * LDBaseVT.getSizeInBits();
+
----------------
Similar to above comment: I think 'getStoreSizeInBits()' is the preferred usage.


Repository:
  rL LLVM

http://reviews.llvm.org/D16217