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

[JF Bastien]

================
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:
----------------
jvoung wrote:
> "We can instead bitcast of the original vector followed by an extract..." sounds a bit weird to me -- missing a few words?
Done.

================
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
----------------
jvoung wrote:
> "If the shuffle is extracting contiguous range of values from..." ->
> "If the shuffle is extracting a contiguous range of values from..." ?
Done.

================
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);
----------------
jvoung wrote:
> Would it be more straightforward to use "Undef = UndefValue::get(Int32Ty)"? 
Good point, done.

================
Comment at: lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1026
@@ +1025,3 @@
+      unsigned SrcElemsPerTgtElem = TgtElemBitWidth / SrcElemBitWidth;
+      assert(SrcElemsPerTgtElem);
+      BegIdx /= SrcElemsPerTgtElem;
----------------
jvoung wrote:
> 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 =(
Yes, it's a sanity check I added in case the code changes and this guarantee is broken. The divide by zero would be caught on x86 anyways, but not on ARM... I'm being overly paranoid on this one :)

================
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.
----------------
jvoung wrote:
> "isn't being replace:"
> ->
> "isn't being replaced:"
Done.

http://reviews.llvm.org/D7734

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