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