[PATCH] D72467: Remove "mask" operand from shufflevector.

[Christopher Tetreault via Phabricator via cfe-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:2544
+    SmallVector<int, 16> IntMask;
+    ShuffleVectorInst::getShuffleMask(cast<Constant>(Mask), IntMask);
+    return CreateShuffleVector(V1, V2, IntMask, Name);
----------------
spatel wrote:
> Add an assert that Mask isa<Constant> ?
cast<T> already does this assert.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:2551
+    SmallVector<int, 16> IntMask;
+    IntMask.assign(Mask.begin(), Mask.end());
+    return CreateShuffleVector(V1, V2, IntMask, Name);
----------------
[0 .. uint32_MAX] is wider than [0 .. int_MAX]. Assert the values are in range somehow?


================
Comment at: llvm/include/llvm/IR/Instructions.h:1985-1986
 ///
+/// The shuffle mask operand specifies, for each element of the result vector,
+/// which element of the two input vectors the result element gets. The
+/// shuffle mask is represented as an array of integers. Positive integers
----------------
spatel wrote:
> This reads awkwardly to me (if you agree, we can update the LangRef too). 
> How about:
> "For each element of the result vector, the shuffle mask selects an element from one of the input vectors to copy to the result. Non-negative elements in the mask represent an index into the concatenated pair of input vectors. UndefMaskElem (-1) specifies that the result element is undefined."
I also found the wording of the description of shufflevector in the language ref confusing. Since this description seems to be related I thought it worth mentioning.

It's unclear to me how the values specify vector inputs. I assume for two vectors <4 x 18>, that the mask <3, 4, 5, 6> takes the last element of the first vector, and the first three elements of the second vector, but this isn't explicitly stated anywhere.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72467/new/

https://reviews.llvm.org/D72467