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

[Stephan Z via Phabricator via llvm-commits]

stephan.yichao.zhao added inline comments.


================
Comment at: llvm/lib/IR/LLVMContextImpl.cpp:147
         if (auto *COp = dyn_cast<ConstantArray>(Op))
           WorkList.insert(COp);
       }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85379