martong 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); ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87519/new/ https://reviews.llvm.org/D87519 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits