[PATCH] D85379: Reduce dropTriviallyDeadConstantArrays cumulative time percentage from 17% to 4%

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.

lgtm



================
Comment at: llvm/lib/IR/LLVMContextImpl.cpp:147
         if (auto *COp = dyn_cast<ConstantArray>(Op))
           WorkList.insert(COp);
       }
----------------
stephan.yichao.zhao wrote:
> tejohnson wrote:
> > Would it make sense to also guard insertion here with COp->use_empty()? You can then remove the C->use_empty() check earlier in this loop, which is redundant for ConstantArrays inserted by the new loop you added above.
> {{F12510614}}
> 
> In the snapshot, 
> 1) an arrow from C1 to C4 means C4 is C1's operand. It seems the 'use' in LLVM means 'reference'. So if an element appears at any side of an arrow, it is 'used'. Please fix me if my assumption is wrong.
> 2) C{i} is ConstantArray. X and Y are other constant like ConstantStruct, ConstantVector, etc
> 
> if we move the use_empty before insert, all C{i} can still be removed the same way. For example, although C4 is not inserted after C1 is deleted before C2 is deleted, after C2 is deleted, C4 can still be inserted.
> 
> The question is if the case of C5 exists. Before C3 is deleted and after C2 is deleted, C5 is still used, so not put into WorkList. When C3 is being deleting, Constant::destroyConstant also tries to delete X. After X is deleted, C5 is not used. We are not able to remove C5 since dropTriviallyDeadConstantArrays does not know this. 
> 
> I am not sure if this can happen. From the [[ http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150105/251381.html | original problem ]] the function to address, such a case, if exists, is not related. Also given our test, in a module with 50k constant arrays, less than 10 operands could be inserted. So it does not change memory reduction rate too much if we move use_empty check close to insert.
> 
> The current code change ensures it removes the same number of ConstantArrays as the previous approach with less time overhead. Maybe we can leave whether insert only when use_empty as the next potential CL with more profiling?
> When C3 is being deleting, Constant::destroyConstant also tries to delete X. After X is deleted, C5 is not used

But could C3 be deleted here if it is still used by X?


However, I see the flaw in what I suggested - presumably all of C's operands are still used before C is destroyed at the bottom of this loop. So none would have use_empty() yet, and so we need to conservatively add them to the worklist in case C is the only use (in which case after C is destroyed they will then have their uses empty). I suppose it could check if there is a single use, and only add it then, but it sounds like it might not be worth it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85379