[PATCH] D94713: Do not traverse ConstantData use-list in SLPVectorizer
Duncan P. N. Exon Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 15 17:23:42 PST 2021
dexonsmith added reviewers: nikic, spatel, bjope, anton-afanasyev.
dexonsmith added a comment.
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.)
================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:989
Value *V = Values[Idx].first;
+ if (isa<ConstantData>(V)) {
+ // Walking the use list of a Constant (particularly, ConstantData)
----------------
willir wrote:
> nikic wrote:
> > Why does this check for ConstantData in particular?
> 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
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.
================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:990-992
+ // 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.
----------------
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.
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94713/new/
https://reviews.llvm.org/D94713
More information about the llvm-commits
mailing list