[PATCH] D39352: [SimplifyCFG] Don't do if-conversion if there is a long dependence chain

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

You have a bunch of variable names here with underscores, which is not our usual convention. `BB1_chain` -> BB1Chain, etc.



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:413
+/// Find out the latency of the longest dependence chain in the BB or the
+/// dependence chain containing the compare instruction.
+static unsigned FindDependenceChainLatency(BasicBlock *BB,
----------------
Please explicitly note in the comment that the 'or' here is determined by the 'LongestChain' parameter.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:497
+
+/// If after if conversion, most of the instructions in new BB construct a
+/// long and slow dependence chain, it may be slower than cmp/branch, even
----------------
in new BB -> in this new BB


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:500
+/// if the branch has a high miss rate, because the data dependence is changed
+/// into control dependence, and the long dependence chain is split into two,
+/// the two parts can be executed in parallel on modern OOO processor.
----------------
I find this sentence confusing. You're trying to say that the control dependence is faster than the data dependence, because the control dependence can be speculated (and thus, the second part can execute in parallel with the first). Right?

This comment should also explain what this is checking. Something like: When considering whether to perform if-conversion, find the length of the dependence chain in BB1 (only the part that can be executed in parallel with code after branch in BB2) before cmp, and if the length is longer than a threshold, don't perform if-conversion." Document what SpeculationSize is.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:518
+  // instructions to fill functional units in small BB.
+  if (New_BB_size > 40)
+    return false;
----------------
Please add a cl::opt for this threshold.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:527
+  // If we have a good ILP (IPC>=2) in new BB, then we don't care about the
+  // latency of the dependence chain.
+  if ((BB1_chain + BB2_chain) * 2 <= New_BB_size)
----------------
You should comment that you're estimating that if there are lots of other instructions in the new BB, they'll be other instructions for the processor to issue regardless of the length of this new dependence chain.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:528
+  // latency of the dependence chain.
+  if ((BB1_chain + BB2_chain) * 2 <= New_BB_size)
+    return false;
----------------
Should this 2 be the SchedModel's IssueWidth?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:539
+  // longer than DependenceChainLatency, then branch is better than select.
+  if (BB1_chain >= DependenceChainLatency)
+    return true;
----------------
Is there a way to think of the proper value of DependenceChainLatency in terms of the branch-misprediction penalty?


https://reviews.llvm.org/D39352