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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/include/llvm/IR/Constants.h:1220
 
+  /// Assert that this is an shufflevector and return the mask. See class
+  /// ShuffleVectorInst for a description of the mask representation.
----------------
an shufflevector -> a shufflevector


================
Comment at: llvm/include/llvm/IR/Constants.h:1224
+
+  /// Assert that this is an shufflevector and return the mask.
+  ///
----------------
an shufflevector -> a shufflevector


================
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);
----------------
Add an assert that Mask isa<Constant> ?


================
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
----------------
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."


================
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) {
----------------
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.


================
Comment at: llvm/include/llvm/IR/PatternMatch.h:1306
+
+struct m_ZeroMask {
+  bool match(ArrayRef<int> Mask) {
----------------
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?


================
Comment at: llvm/include/llvm/IR/PatternMatch.h:1320
+
 /// Matches ShuffleVectorInst.
+template <typename V1_t, typename V2_t>
----------------
Update/add comment:
"Matches ShuffleVectorInst independently of mask value." ?


================
Comment at: llvm/lib/Bitcode/Writer/ValueEnumerator.cpp:937-939
+   if (auto *CE = dyn_cast<ConstantExpr>(C))
+     if (CE->getOpcode() == Instruction::ShuffleVector)
+       EnumerateOperandType(CE->getShuffleMaskForBitcode());
----------------
Indentation looks off-by-1.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1937
+  ArrayRef<int> Mask;
+  if (const ShuffleVectorInst *SVI = dyn_cast<ShuffleVectorInst>(&U))
+    Mask = SVI->getShuffleMask();
----------------
if (auto *SVI = ...)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3587
+  ArrayRef<int> Mask;
+  if (const ShuffleVectorInst *SVI = dyn_cast<ShuffleVectorInst>(&I))
+    Mask = SVI->getShuffleMask();
----------------
if (auto *SVI = ...)


================
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)
----------------
I've never looked in here before - what happens currently if the index is -1 (undef)?


================
Comment at: llvm/lib/IR/ConstantsContext.h:175
+
   // allocate space for exactly three operands
   void *operator new(size_t s) {
----------------
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 "two operands" and a proper sentence with a capital letter and period.


================
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:
----------------
(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.


================
Comment at: llvm/lib/IR/Instruction.cpp:437
            RMWI->getSyncScopeID() == cast<AtomicRMWInst>(I2)->getSyncScopeID();
+  if (const ShuffleVectorInst *SVI = dyn_cast<ShuffleVectorInst>(I1))
+    return SVI->getShuffleMask() == cast<ShuffleVectorInst>(I2)->getShuffleMask();
----------------
Would have suggested "auto *" here, but really the whole thing should be turned into a switch() as a preliminary cleanup?


================
Comment at: llvm/lib/IR/Instructions.cpp:1824-1825
+                                     Instruction *InsertBefore)
+: Instruction(VectorType::get(cast<VectorType>(V1->getType())->getElementType(),
+                Mask.size(), V1->getType()->getVectorIsScalable()),
+              ShuffleVector,
----------------
I see this is copied, but indentation seems off.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:304
       e.varargs.push_back(*II);
+  } else if (ShuffleVectorInst *SVI = dyn_cast<ShuffleVectorInst>(I)) {
+    ArrayRef<int> ShuffleMask = SVI->getShuffleMask();
----------------
Use "auto *" in each clause (or convert to switch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72467