[PATCH] D85379: Improve dropTriviallyDeadConstantArrays time cost ratio from 17% to 4%

[Stephan Z via Phabricator via llvm-commits]

stephan.yichao.zhao added a comment.

In D85379#2198662 <https://reviews.llvm.org/D85379#2198662>, @ychen wrote:

> What is time cost ratio?

updated the description.



================
Comment at: llvm/lib/IR/LLVMContextImpl.cpp:149
+  // the initial pass. So the sizes of WorkList and Deleted are much smaller
+  // than the size of ArrayConstants.
+  SmallPtrSet<ConstantArray *, 4> Deleted;
----------------
ychen wrote:
> jdoerfert wrote:
> > Please proof read this comment.
> I'd go with "When ArrayConstants are of substantial size and only a few in them is dead, starting WorkList with all elements of ArrayConstants can be wasteful. Instead, starting WorkList with only elements that have empty uses."
Rewrote comments after simplifying the code.

Also removed cl/324220676 from the summary. It is not related. Uploaded some pprof results.


================
Comment at: llvm/lib/IR/LLVMContextImpl.cpp:161
+    C->destroyConstant();
+    Deleted.insert(C);
+  }
----------------
mehdi_amini wrote:
> It actually isn't clear to why you need the `Deleted` set at all.
> 
> Can't we just use this loop to prime the worklist with the entries that aren't used and then process the worklist below?
> 
> ```
> for (ConstantArray *Constant : ArrayConstants) {
>   if (C->use_empty())
>     WorkList.insert.COp);
> }
> 
> while (!WorkList.empty()) {
>   // unchanged...
> ```
> 
Thank you. Removed "Deleted" and applied your suggested code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85379/new/

https://reviews.llvm.org/D85379