[PATCH] D66718: [DAGCombiner] (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), IdxC) -> (vector_shuffle X, Y)
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Aug 25 11:05:10 PDT 2019
- Previous message: [PATCH] D66718: [DAGCombiner] (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), IdxC) -> (vector_shuffle X, Y)
- Next message: [PATCH] D66718: [DAGCombiner] (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), IdxC) -> (vector_shuffle X, Y)
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
lebedev.ri added inline comments.
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16432-16433
+
+ // (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), IdxC)
+ // --> (vector_shuffle X, Y)
+ if (Vec.getOpcode() == ISD::VECTOR_SHUFFLE && Vec.hasOneUse() &&
----------------
Do you check that both vector_shuffle and extract_vector_elt have the same first argument?
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16434-16436
+ if (Vec.getOpcode() == ISD::VECTOR_SHUFFLE && Vec.hasOneUse() &&
+ InsertVal.getOpcode() == ISD::EXTRACT_VECTOR_ELT &&
+ isa<ConstantSDNode>(InsertVal.getOperand(1))) {
----------------
Here we have two instructions, and only require that the first one is single-use.
Observation: we only create a single new instruction.
Do we want to either require both of them be single-use, the first one, either one of them, or none of them?
Another observation - you end up keeping (using) the instruction you just checked for extra uses.
Did you mean to check the other one instead?
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16448-16450
+ SmallVector<int, 16> NewMask(Mask.size());
+ for (size_t i = 0; i < Mask.size(); i++)
+ NewMask[i] = Mask[i];
----------------
So `NewMask` is simply inited as a copy of `SVN->getMask()`?
Would something like `SmallVector<int, 16> NewMask(SVN->getMask());` just work?
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16452
+
+ auto *IndexC = dyn_cast<ConstantSDNode>(InsertVal.getOperand(1));
+ NewMask[InsIndex] = Offset + IndexC->getZExtValue();
----------------
You either need to check that the cast succeeded, or drop `dyn_` prefix
================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16453
+ auto *IndexC = dyn_cast<ConstantSDNode>(InsertVal.getOperand(1));
+ NewMask[InsIndex] = Offset + IndexC->getZExtValue();
+
----------------
This could use a comment
================
Comment at: test/CodeGen/X86/madd.ll:1901-1903
-; AVX512-NEXT: vpextrd $3, %xmm0, %eax
-; AVX512-NEXT: vpshufd {{.*#+}} xmm0 = xmm0[1,1,2,3]
-; AVX512-NEXT: vpinsrd $1, %eax, %xmm0, %xmm0
----------------
If i'm reading this correctly this does showcase the need to handle this in backend - it can be introduced there?
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66718/new/
https://reviews.llvm.org/D66718
- Previous message: [PATCH] D66718: [DAGCombiner] (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), IdxC) -> (vector_shuffle X, Y)
- Next message: [PATCH] D66718: [DAGCombiner] (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), IdxC) -> (vector_shuffle X, Y)
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
More information about the llvm-commits
mailing list