We can avoid repetitive calls getSameOpcode() for already known tree elements by keeping MainOp and AltOp in TreeEntry. Also, ScalarToTreeEntry key should be at this point not just Value but Instruction.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
1393 ↗ | (On Diff #209711) | Please add set/get methods and make these member variables private. |
1395 ↗ | (On Diff #209711) | Also please improve TreeEntry's dump() so that it prints whether this is an alt sequence entry. You could also perhaps print these values if needed. |
1473 ↗ | (On Diff #209711) | const? |
1520 ↗ | (On Diff #209711) | I was under the impression that we are completely removing InstructionsState and replacing it with state from TreeEntry. |
1564 ↗ | (On Diff #209711) | Hmm isn't this an unrelated change to this patch? |
1572 ↗ | (On Diff #209711) | Same here |
1580 ↗ | (On Diff #209711) | Same |
2384 ↗ | (On Diff #209711) | Shouldn't we be getting this from the TreeEntry? Something like E->isAltShuffle() |
2976 ↗ | (On Diff #209711) | This also looks unrelated to this patch. |
3565 ↗ | (On Diff #209711) | It would be nice to have a method to hide this. Perhaps E->getSchedulingDataKey() or something along those lines. |
4004 ↗ | (On Diff #209711) | Why do we still have the InstructionsState here? Shouldn't we use data from the TreeEntry instead? |
4156 ↗ | (On Diff #209711) | I think it would be nice to have TreeEntry methods E->getMainOpcode() and E->getAltOpcode(). |
Rebased, fixed previous remarks. Here we still have InstructionsState and I have the full change with the state in TreeEntry but I prefer to break it in several reviews.
Just a couple more comments.
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
2790 ↗ | (On Diff #211907) | How about this getSameOpcode() ? Can't we replace this too? |
3833 ↗ | (On Diff #211907) | Hmm, this block starting at line 3828 looks a bit redundant now. Shouldn't this state update be done internally within TreeEntry? How about using something like E->updateStateIfReorder() ? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
1264 ↗ | (On Diff #213286) | Remove extra ; |
1283 ↗ | (On Diff #213286) | const reference? |
1305 ↗ | (On Diff #213286) | Description? |
1356 ↗ | (On Diff #213286) | const reference? |
3836–3837 ↗ | (On Diff #213286) | Better to keep IsReorder in the condition and change its initial value to E->updateStateIfReorder() |
2976 ↗ | (On Diff #209711) | Not fixed |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
1572 ↗ | (On Diff #209711) | These NFC changes are still here - remove from this patch? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
1572 ↗ | (On Diff #209711) | hmm, We can refer to an entry by a pointer instead of a number and I prefer to keep it unless the other opinion? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
2823 ↗ | (On Diff #214703) | Seems to me, it is not formatted |
3577 ↗ | (On Diff #214703) | Format? |
3837 ↗ | (On Diff #214703) | Why not: bool IsReorder = E->updateStateIfReorder(); if (IsReorder) .... |
1283 ↗ | (On Diff #213286) | Not done. |
1305 ↗ | (On Diff #213286) | Use /// style of comments here |
1356 ↗ | (On Diff #213286) | Not done |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
1308 ↗ | (On Diff #215117) | Clang-format? |
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
1572 ↗ | (On Diff #209711) | In which case I'd recommend just doing this as a NFC commit now to avoid it from this patch. |