[PATCH] D11720: [ARM] Update ReconstructShuffle to handle mismatched types

[Renato Golin]

rengolin added a comment.

Hi Silviu,

This is a good patch, thanks! I have a few comments inline, but it's mostly good.

About the test, you seem to cover a number of new cases and only one is being tested. I'd like to see similar functions with undefs on either places, and also wrongly sized vectors not being transformed, to make sure we're not breaking anything.

cheers,
--renato


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:5606
@@ +5605,3 @@
+    EVT SrcEltTy = Source.Vec.getValueType().getVectorElementType();
+    if (SrcEltTy.bitsLT(SmallestEltTy)) {
+      SmallestEltTy = SrcEltTy;
----------------
extra curly braces

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:5640
@@ -5612,6 +5639,3 @@
 
-    // Since only 64-bit and 128-bit vectors are legal on ARM and
-    // we've eliminated the other cases...
-    assert(SourceVecs[i].getValueType().getVectorNumElements() == 2*NumElts &&
-           "unexpected vector sizes in ReconstructShuffle");
+    assert(SrcVT.getSizeInBits() == 2 * VT.getSizeInBits());
 
----------------
It's obviously better to have hard rules, where they're needed, but what if the vector types are odd or larger than expected. Shouldn't we return SDValue() here, instead of blowing up?

Same on the assert just above.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:5683
@@ +5682,3 @@
+    assert(ShuffleVT.getVectorElementType() == SmallestEltTy);
+    Src.ShuffleVec = DAG.getNode(ISD::BITCAST, dl, ShuffleVT, Src.ShuffleVec);
+    Src.WindowScale = SrcEltTy.getSizeInBits() / SmallestEltTy.getSizeInBits();
----------------
I assume that getNode will fail miserably if the bitcast is not possible / allowed. But I can't think of anything that could go wrong at this stage regarding casts...

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:5689
@@ +5688,3 @@
+  // Final sanity check before we try to actually produce a shuffle.
+  DEBUG(
+    for (auto Src : Sources)
----------------
Is DEBUG also on during Release+Asserts? This is how most of the buildbots run nowadays, so it'd be good to add this check when in Release mode, too.

I'm assuming that, if you remove the DEBUG(), under O3, the compiler will remove the whole loop if the asserts fail to materialise.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:5694
@@ +5693,3 @@
+
+  // The stars all align, our next step is to produce the mask for the shuffle.
+  SmallVector<int, 8> Mask(ShuffleVT.getVectorNumElements(), -1);
----------------
Love this comment! :)

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:5727
@@ -5673,2 +5726,3 @@
 
-  return SDValue();
+  SDValue ShuffleOps[] = { DAG.getUNDEF(ShuffleVT), DAG.getUNDEF(ShuffleVT) };
+  for (unsigned i = 0; i < Sources.size(); ++i)
----------------
This seems dangerous. Assuming there are only two, then using Sources.size().

I know there is an assert right at the beginning, but this line of code wouldn't know if that number has changed since, or if the assert is removed at a later date.

I'd put another assert here, too.


http://reviews.llvm.org/D11720