[PATCH] D87519: [analyzer][Liveness][NFC] Enqueue the CFGBlocks post-order

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: clang/lib/Analysis/LiveVariables.cpp:522
 
-  // FIXME: we should enqueue using post order.
-  for (const CFGBlock *B : cfg->nodes()) {
+  for (const CFGBlock *B : *AC.getAnalysis<PostOrderCFGView>()) {
     worklist.enqueueBlock(B);
----------------
martong wrote:
> xazax.hun wrote:
> > With `BackwardDataflowWorklist`, each  `enqueueBlock` will insert the block into a `llvm::PriorityQueue`. So regardless of the insertion order, `dequeue` will return the nodes in the reverse post order. 
> > 
> > Inserting elements in the right order into the heap might be beneficial is we need to to less work to "heapify". But on the other hand, we did more work to insert them in the right order, in the first place. All in all, I am not sure whether the comment is still valid and whether this patch would provide any benefit over the original code. 
> > 
> Yes, what Gabor says makes sense. On the other hand I don't see any overhead - I might be wrong though - in the post order visitation. And it makes the code more consistent IMHO.
> 
> Well, it would be important to know why the original author put the 
> ```
> // FIXME: we should enqueue using post order.
> ```
> there. The blamed commit 77ff930fff15c3fc76101b38199dad355be0866b is not saying much.
Please compare the execution time with and without this patch. I think it is difficult do decide in theory which one costs more: the heapification during insertion or the reverse ordering before the insertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87519