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

[silviu.baranga at arm.com]

sbaranga added a comment.

Hi Renato,

Thanks for the review!

FWIW I've tested this with lnt, spec2000, and some other benchmarks, and everything seems to work.
Getting code to trigger in this stage of codegen is always tricky...

Cheers,
Silviu


================
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());
 
----------------
rengolin wrote:
> 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.
Yes, good catch! I believe you're correct. I haven't managed to trigger this behaviour from IR though.

================
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)
----------------
rengolin wrote:
> 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.
Looks like DEBUG is always a nop in non-debug builds. I'll remove the DEBUG statement.

================
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)
----------------
rengolin wrote:
> 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.
The code itself should be safe (we're not adding more sources from the point where we're checking that we have at most two), but I agree that an assert here is a good idea.


http://reviews.llvm.org/D11720