[PATCH] D94713: Do not traverse ConstantData use-list in SLPVectorizer

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

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