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

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
1193

Please add set/get methods and make these member variables private.

1195

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.

1273

const?

1356

I was under the impression that we are completely removing InstructionsState and replacing it with state from TreeEntry.

1399–1401

Hmm isn't this an unrelated change to this patch?

1406–1408

Same here

1413

Same

2217

Shouldn't we be getting this from the TreeEntry? Something like E->isAltShuffle()

2809–2810

This also looks unrelated to this patch.

3399

It would be nice to have a method to hide this. Perhaps E->getSchedulingDataKey() or something along those lines.

3836–3837

Why do we still have the InstructionsState here? Shouldn't we use data from the TreeEntry instead?

3985

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
1356

yes, I think the same.

1399–1401

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

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
2799

How about this getSameOpcode() ? Can't we replace this too?

3839

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

Remove extra ;

1283

const reference?

1305

Description?

1356

const reference?

2809–2810

Not fixed

3836–3837

Better to keep IsReorder in the condition and change its initial value to E->updateStateIfReorder()

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
1406–1408

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
1406–1408

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
1283

Not done.

1305

Use /// style of comments here

1356

Not done

2824

Seems to me, it is not formatted

3578

Format?

3836–3837

Why not:

bool IsReorder = E->updateStateIfReorder();
if (IsReorder)
....

Fixed remarks.

xbolva00 added inline comments.
lib/Transforms/Vectorize/SLPVectorizer.cpp
1308

Clang-format?

fixed formatting issue at line 1308.

RKSimon added inline comments.Aug 14 2019, 10:12 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
1406–1408

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