[PATCH] D13363: [DeadStoreElimination] Add support for DSE across blocks

[Ivan Baev via llvm-commits]

ivanbaev updated this revision to Diff 36999.
ivanbaev added a comment.

Addressed Erik's comments:
*) Your algorithm has quadratic complexity in worst case. I know this is the same as for the single block algorithm, but extending it to multiple blocks might worsen the situation. Maybe you add some sort of upper bound for the number of dependency checks?

- An upper bound (MaxNonLocalAttempts = 100) is added on the number of non-local DSE attempts per function.

*) Your algorithm only handles certain types of cross-block DSE. As I understood, it cannot go beyond an if-then-else control flow split. I'm wondering if it makes sense to do a simple data flow analysis to collect the eligible predecessor blocks. This would handle all cases and I think it would be not much more implementation effort and doesn't add compile time.

- Analyzing general loops is challenging and compile-time extensive due to underlying MemoryDependenceAnalysis. This patch iteratively goes through predecessors that unconditionally lead to the block with the store and search for redundant stores. Not sure if there are opportunities within a reasonable compile-time budget for a truly global DSE.

*) I found it confusing that all the code is contained in this large functions with many nested loops and breaks and continues. I think it's better to extract some of the code to separate functions (e.g. to avoid the StopProcessPBAndItsPreds variable).

- The code is refactored with top algorithm extracted in HandleNonLocalDependency(Instruction *Inst).

*) The tests look like they contain much code which is not really necessary to test what you want to test. Maybe you can reduce them. Also in the first test a comment would help, which describes why no DSE can be done.

- I reduced the cycle test to a much smaller version, and added comments.


http://reviews.llvm.org/D13363

Files:
  lib/Transforms/Scalar/DeadStoreElimination.cpp
  test/Transforms/DeadStoreElimination/cycle.ll
  test/Transforms/DeadStoreElimination/ifthen.ll
  test/Transforms/DeadStoreElimination/loop.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D13363.36999.patch
Type: text/x-patch
Size: 18830 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151009/13e43f20/attachment.bin>