[PATCH] D54267: [X86][SSE] Add lowerVectorShuffleAsByteRotateAndPermute (PR39387)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:10495
+  // can be widened.
+  if (!(0 <= Range1.first && Range1.second < NumElts &&
+        Range1.first <= Range1.second) ||
----------------
craig.topper wrote:
> Should this be NumLaneElts instead of NumElts?
I think this comment was addressed?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:10496
+  if (!(0 <= Range1.first && Range1.second < NumElts &&
+        Range1.first <= Range1.second) ||
+      !(0 <= Range2.first && Range2.second < NumElts &&
----------------
craig.topper wrote:
> If the min/max is inside the lane, isn't Range1.first always less than or equal to Range1.second? And isn't Range1.first always less than or equal to Range1.second unless the input is unused by the shuffle? Which would mean the input is undef?
I dont' think i saw an answer for this


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:10170
+  unsigned EltSize = VT.getScalarSizeInBits();
+  if (ImmBlends && EltSize == 8 && !canWidenShuffleElements(BlendMask))
+    return SDValue();
----------------
If the elements could be widened wouldn't they have already been widened in lowerVectorShuffle?


Repository:
  rL LLVM

https://reviews.llvm.org/D54267