nikic added a comment.

I'm somewhat skeptical about this change. The motivation seems a bit weak given 
the costs involved. The costs are:

- Compile-time cost. Your best case estimate puts this at a 0.5% compile-time 
regression. This is for the case where SimplifyCFG simplify preserves DT, 
without using it. Once the DT is used, the DTU may be flushed many times while 
SimplifyCFG iterates. This will drive the average-case cost up, and likely 
introduce pathological cases as well.
- Code complexity: At least watching from afar, your path to preserving DT in 
SimplifyCFG involved quite a few subtle issues, where the first implementation 
of DT preservation for a given transformation later turned out not to be 
entirely correct. Given the large number of very different transforms that 
SimplifyCFG performs, this adds a code complexity and maintenance cost.

The (listed) benefits are:

- Not processing unreachable blocks. I am doubtful that we'll want to use the 
DT for this purpose, because that would require flushing the DTU on each 
SimplifyCFG iteration. You'd probably be better off running 
removeUnreachableBlocks each time. Though it's not clear that this is even a 
problem that needs solving.
- Using DT in FoldBranchToCommonDest. This seems to be the real motivation 
here. I will have to dive deeper into the code to understand what the problem 
here is and whether it justifies the costs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94827

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to