This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
ymandel marked an inline comment as done.
Closed by commit rG26db5e651bb6: [clang][CFG] Support construction of a weak
topological ordering of the CFG. (authored by
ymandel marked 5 inline comments as done.
ymandel added a comment.
Thanks for the helpful suggestions!
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:104
+/// intervals) if and only if it is reducible (its limit flow graph has one
+/// node). Returns
ymandel updated this revision to Diff 544551.
ymandel marked 2 inline comments as done.
ymandel added a comment.
Addressed comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
sammccall accepted this revision.
sammccall added a comment.
Thanks, this is much simpler!
Just nits & apologies for the delay.
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:24
#include "llvm/ADT/DenseSet.h"
+#include
+#include
some
xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.
Thanks, looks good to me!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
ymandel added inline comments.
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:101-103
+/// groups topologically. As a result, the blocks in a series of loops are
+/// ordered such that all nodes in loop `i` are earlier in the order than nodes
+/// in loop
ymandel updated this revision to Diff 543552.
ymandel marked 2 inline comments as done.
ymandel added a comment.
Addressed comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
xazax.hun added inline comments.
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:122
+
+ using BlockOrderTy = llvm::DenseMap;
+ BlockOrderTy BlockOrder;
Would it make sense to have a vector instead and use block ids to get the order?
ymandel added a comment.
Thanks for the very helpful feedback!
I think I've addressed all of the suggestions/concerns. I suspect there's yet
more room for improvement in the algorithm, but I think we're converging on
"good enough".
Comment at:
ymandel updated this revision to Diff 541153.
ymandel marked 12 inline comments as done.
ymandel added a comment.
tweaks
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
ymandel updated this revision to Diff 541141.
ymandel added a comment.
Radically simplify the code, per comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
sammccall added a comment.
Having (mostly!) understood what this is doing, the algorithm looks like it's
doing the right thing.
High-level ideas would be:
- there are three different representations/sets of APIs exposed: intervals,
ordering, and compare. It seems like we only have immediate
xazax.hun added inline comments.
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:56-64
+ template struct NodeData {
+// The block from which the interval was constructed. It is either the
+// graph's entry block or has at least one predecessor
ymandel marked 2 inline comments as done.
ymandel added inline comments.
Comment at: clang/lib/Analysis/IntervalPartition.cpp:121
+Index.emplace(N, ID);
+ Graph.Intervals.emplace_back(ID, Header, std::move(Data.Nodes));
}
xazax.hun wrote:
> It would
xazax.hun added inline comments.
Comment at: clang/lib/Analysis/IntervalPartition.cpp:121
+Index.emplace(N, ID);
+ Graph.Intervals.emplace_back(ID, Header, std::move(Data.Nodes));
}
It would probably take for me some time to understand what is going on
xazax.hun added inline comments.
Comment at: clang/unittests/Analysis/IntervalPartitionTest.cpp:21
+namespace {
+template using NodeData = CFGInterval::NodeData;
+} // namespace
Another redundant anonymous namespace here.
Repository:
rG LLVM Github
ymandel marked 4 inline comments as done.
ymandel added a comment.
Rebased, thanks!
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:73
+
+ // Whether this node is the head of a feedback edge within the interval.
+ bool IsFeedbackHead = false;
ymandel updated this revision to Diff 535402.
ymandel added a comment.
Address more comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
ymandel updated this revision to Diff 535397.
ymandel added a comment.
Rebase and address some comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
xazax.hun added a comment.
Could you rebase this patch to the latest tip of tree?
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:73
+
+ // Whether this node is the head of a feedback edge within the interval.
+ bool IsFeedbackHead = false;
ymandel updated this revision to Diff 531874.
ymandel added a comment.
more comment expansion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
ymandel updated this revision to Diff 531865.
ymandel added a comment.
Fix build break and add some field comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
ymandel updated this revision to Diff 531843.
ymandel added a comment.
fix some comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
Files:
ymandel created this revision.
ymandel added reviewers: xazax.hun, gribozavr.
Herald added a reviewer: NoQ.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.
This patch adds support for building a weak topological ordering of the CFG
blocks,
24 matches
Mail list logo
