This is an archive of the discontinued LLVM Phabricator instance.

[SLPVectorizer] [NFC] Avoid repetitive calls to getSameOpcode().
ClosedPublic

Authored by dtemirbulatov on Jul 13 2019, 7:24 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

dtemirbulatov created this revision.Jul 13 2019, 7:24 PM
vporpo added inline comments.Jul 15 2019, 11:53 AM
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().

dtemirbulatov marked 2 inline comments as done.Jul 15 2019, 3:46 PM
dtemirbulatov added inline comments.
lib/Transforms/Vectorize/SLPVectorizer.cpp
1520 ↗(On Diff #209711)

yes, I think the same.

1564 ↗(On Diff #209711)

Hmm, I noticed that @ABataev in D57059 uses Value* to UndefValue to handle empty operations and I think now that ScalarToTreeEntry should be Value* type key as it is now, so we don't need change.

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.

dtemirbulatov marked 8 inline comments as done.Jul 26 2019, 3:48 AM
RKSimon added a subscriber: Vasilis.EditedAug 4 2019, 6:30 AM

one minor - @vporpo @ABataev any comments?

lib/Transforms/Vectorize/SLPVectorizer.cpp
1270 ↗(On Diff #211907)

Can this ever get called when MainOp or AltOp is null? You test for that case below in getOpcode/getAltOpcode

vporpo added a comment.Aug 4 2019, 1:07 PM

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() ?

Fixed all previous remarks, rebased.

dtemirbulatov marked 4 inline comments as done.Aug 5 2019, 12:59 AM

@ABataev are you OK with this?

ABataev added inline comments.Aug 8 2019, 9:38 AM
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

Fixed remarks.

dtemirbulatov marked 6 inline comments as done.Aug 12 2019, 2:09 PM
RKSimon added inline comments.Aug 14 2019, 6:00 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
1572 ↗(On Diff #209711)

These NFC changes are still here - remove from this patch?

dtemirbulatov marked an inline comment as done.Aug 14 2019, 6:12 AM
dtemirbulatov added inline comments.
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?

ABataev added inline comments.Aug 14 2019, 6:24 AM
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

Fixed remarks.

xbolva00 added inline comments.
lib/Transforms/Vectorize/SLPVectorizer.cpp
1308 ↗(On Diff #215117)

Clang-format?

fixed formatting issue at line 1308.

RKSimon added inline comments.Aug 14 2019, 10:12 AM
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.

Rebase please

This revision is now accepted and ready to land.Aug 19 2019, 6:57 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2019, 5:21 PM