This is an archive of the discontinued LLVM Phabricator instance.

Do not traverse ConstantData use-list in SLPVectorizer
ClosedPublic

Authored by willir on Jan 14 2021, 12:34 PM.

Details

Summary

Walking the use list of a Constant (particularly, ConstantData)
is not scalable, since a given constant may be used by many
instructinos in many functions in many modules.

Diff Detail

Event Timeline

willir created this revision.Jan 14 2021, 12:34 PM
willir requested review of this revision.Jan 14 2021, 12:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 12:34 PM
nikic added a subscriber: nikic.Jan 14 2021, 12:42 PM
nikic added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
990

Why does this check for ConstantData in particular?

willir added inline comments.Jan 15 2021, 4:21 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
990

Hi, thanks for the review.

Instances of ConstantData are unique for the entire an LLVMContext, so its uses can be spread across multiple functions and modules.
This commit is a part of a bigger project to remove use-list from ConstantData.

Here are some links for more info about it:
https://bugs.llvm.org/show_bug.cgi?id=30513
https://lists.llvm.org/pipermail/llvm-dev/2016-September/105157.html

Adding some reviewers that have touched the SLPVectorizer recently to check this. (I have context on the motivation for the change and planned changes to the IR, but I don't know this pass well.)

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
990

It's possible that rejecting all Constants makes sense here, depending on whether this code needs to walk through GlobalValue-derived constants. I suspect it doesn't and we can just do Constant.

991–993

I think this comment will be more clear if it highlights the semantic problem. I suggest something like:

// Since this is a function pass, it doesn't make semantic sense to walk the
// users of a subclass of Constant. The users could be in another function,
// or even another module that happens to be in the same LLVMContext.
willir updated this revision to Diff 317218.Jan 17 2021, 7:23 AM

Correct comment explaining the reason to skip ConstantData, as suggested by
@dexonsmith. At the moment isa<ConstantData>, I'm not sure whether using
isa<Constant> whould be better here.

You are planning to revert this patch after ConstantData use-list removing, am I right?

spatel added inline comments.Jan 18 2021, 7:41 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
990

That sounds right to me - this pass is only concerned with "basic" integer/FP constants. For example, we have allConstant() around line 219:

/// \returns True if all of the values in \p VL are constants (but not
/// globals/constant expressions).

So I think it's safe to ignore any Constant here.

You are planning to revert this patch after ConstantData use-list removing, am I right?

Hi, well, not really, after ConstantData use-list removed, we definitely cannot walk its use-list.
We could change the check from isa to calling hasUseList(), but I'm not sure if it would be more readable here.

willir updated this revision to Diff 317441.Jan 18 2021, 5:31 PM

Ignore all Constants, not just ConstantData

spatel accepted this revision.Jan 19 2021, 4:56 AM

I have not followed the plans for changes to the IR, etc. But this doesn't seem to conflict with any optimizations that SLP tries to do, so LGTM.

This revision is now accepted and ready to land.Jan 19 2021, 4:56 AM

I don't have commit access. Would someone please commit for me?

  • GIT_AUTHOR_NAME="Anton Rapetov"
  • GIT_AUTHOR_EMAIL="willir29@gmail.com"
This revision was automatically updated to reflect the committed changes.