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

[Eli Friedman via Phabricator via cfe-commits]

efriedma marked 5 inline comments as done.
efriedma added inline comments.


================
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);
----------------
ctetreau wrote:
> [0 .. uint32_MAX] is wider than [0 .. int_MAX]. Assert the values are in range somehow?
Either the resulting shuffle mask is invalid (and we'll assert later), or the input contained UINT_MAX, in which case we'll treat it as undef.


================
Comment at: llvm/include/llvm/IR/Instructions.h:2015
 
-  // allocate space for exactly three operands
+  // allocate space for exactly two operands
   void *operator new(size_t s) {
----------------
spatel wrote:
> This comment doesn't add value to me, so I'd just delete it. If we want to keep it, should fix it to be a proper sentence with a capital letter and period.
I think the comment is there to identify the magic number.  Maybe I should introduce a named constant ShuffleVectorInst::NumOperands to make that clear.


================
Comment at: llvm/include/llvm/IR/PatternMatch.h:1306
+
+struct m_ZeroMask {
+  bool match(ArrayRef<int> Mask) {
----------------
spatel wrote:
> IIUC, this is meant to replace the current uses of m_Zero() or m_ZeroInt() with shuffle pattern matching, but there's a logic difference because those matchers allow undefs, but this does not? Are we missing unittests to catch that case?
I guess we're missing tests, yes; I didn't see any failures.


================
Comment at: llvm/lib/ExecutionEngine/Interpreter/Execution.cpp:1890
       for( unsigned i=0; i<src3Size; i++) {
-        unsigned j = Src3.AggregateVal[i].IntVal.getZExtValue();
+        unsigned j = std::max(0, I.getMaskValue(i));
         if(j < src1Size)
----------------
spatel wrote:
> I've never looked in here before - what happens currently if the index is -1 (undef)?
The interpreter evaluates undef to zero, and therefore it behaves as if every undef is replaced with zero.

Using std::max here preserves the existing behavior.


================
Comment at: llvm/lib/IR/ConstantsContext.h:564
     case Instruction::ShuffleVector:
-      return new ShuffleVectorConstantExpr(Ops[0], Ops[1], Ops[2]);
+      return new ShuffleVectorConstantExpr(Ops[0], Ops[1], Indexes);
     case Instruction::InsertValue:
----------------
spatel wrote:
> (not familiar with this part)
> If we're using Indexes here, do we need to update the code/comments for hasIndices()?
> 
>   /// Return true if this is an insertvalue or extractvalue expression,
>   /// and the getIndices() method may be used.
As part of constructing ConstantExprs, there's a map from ConstantExprKeyType to ConstantExpr, to ensure each expression is unique.  "Indexes" is just a list of integers, so I was reusing it as a shortcut, to try to avoid making an extra ArrayRef.  This only impacts the map itself, not the final ConstantExprs.

It looks like I missed that a few places use `hasIndices()` to check whether the ConstantExprKeyType key stores data in Indexes.  I should probably just add the extra member variable to make the logic clear, even if it wastes a tiny bit of stack space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72467