[PATCH] InstCombine: extract instead of shuffle when performing vector/array type punning

[Jan Voung]

Mostly nits, but looks okay to me.


================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:955
@@ +954,3 @@
+  // SROA generates shuffle+bitcast when the extracted sub-vector is bitcast to
+  // a non-vector type. We can instead bitcast of the original vector followed
+  // by an extract of the desired element:
----------------
"We can instead bitcast of the original vector followed by an extract..." sounds a bit weird to me -- missing a few words?

================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:965
@@ +964,3 @@
+  //
+  // If the shuffle is extracting contiguous range of values from the input
+  // vector then each use which is a bitcast of the extracted size can be
----------------
"If the shuffle is extracting contiguous range of values from..." ->
"If the shuffle is extracting a contiguous range of values from..." ?

================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1016
@@ +1015,3 @@
+        // [NumElems,SrcNumElems) is undef.
+        Constant *Undef = ConstantInt::get(Int32Ty, SrcNumElems);
+        SmallVector<Constant *, 16> ShuffleMask(SrcNumElems, Undef);
----------------
Would it be more straightforward to use "Undef = UndefValue::get(Int32Ty)"? 

================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1026
@@ +1025,3 @@
+      unsigned SrcElemsPerTgtElem = TgtElemBitWidth / SrcElemBitWidth;
+      assert(SrcElemsPerTgtElem);
+      BegIdx /= SrcElemsPerTgtElem;
----------------
Is there anything that actually guarantees TgtElemBitWidth > SrcElemBitWidth? Could be safer to just check this earlier and bail?

N/M (this should be guaranteed by the shuffled vector have >= 1 elems), but phabricator won't let me delete this comment =(

================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1037
@@ +1036,3 @@
+          NewBC, ConstantInt::get(Int32Ty, BegIdx), SVI.getName() + ".extract");
+      // The shufflevector isn't being replace: the bitcast that used it
+      // is. InstCombine will visit the newly-created instructions.
----------------
"isn't being replace:"
->
"isn't being replaced:"

http://reviews.llvm.org/D7734

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/