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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits