[PATCH] D10979: Update the branch weight metadata in JumpThreading pass.
Cong Hou
congh at google.com
Tue Jul 21 13:50:08 PDT 2015
congh marked an inline comment as done.
================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1578
@@ +1577,3 @@
+ // distributed to CFG nodes. In this case we just set the new frequency of
+ // BB to zero.
+ uint64_t NewBBFreq = (PredBBFreq <= BBFreq)
----------------
davidxl wrote:
> Is this still an issue that needs the workaround here?
Yes. The issue emerges after jump-threading + loop-rotation + jump-threading will lead to this.
================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1596
@@ +1595,3 @@
+ "The weight on BB->SuccBB is not coherent with it in BPI");
+ if (BB2SuccBBFreq.getFrequency() > 0) {
+ DEBUG(dbgs() << " Update the weight on the edge from "
----------------
davidxl wrote:
> Why this condition?
I forget to add a comment when I did this. But I think this condition can be removed now.
================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1599
@@ +1598,3 @@
+ << BB->getName() << " to " << SuccBB->getName() << "\n");
+ if (PredBBFreq <= BB2SuccBBFreq) {
+ // Here we record the index of SuccBB in successors of BB and
----------------
davidxl wrote:
> It is unclear how can this happen.
Consider the following CFG and assume the edge frequency is 100 for each edge:
```
A B
\ /
C
/ \
D E
```
In some pass C may be split into two blocks:
```
A B
| |
C1 C2
| X |
D E
```
Then edges from C1/C2 have frequency 50. Now suppose in jump-threading pass we thread B->E across C2 (C2->D is impossible at runtime), then B has frequency 100 but C2->E has frequency 50. This is when this condition is not met.
================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1614
@@ +1613,3 @@
+ if (SuccIndexToUpdate >= 0) {
+ // We need to update the weights of BB's successors.
+ // Instead of scaling the weight of BB->SuccBB by 1 - Freq(PredBB->BB) /
----------------
davidxl wrote:
> The scaling code looks unnecessarily complicated. How about this:
>
> a) Pre-normalization
> 1) first sum up all the weights before the update
> 2) if the sum is too small, scale it up to a fixed val such as ScaledNumber<64>(1,20)
> 3) compute the scale factor and rescale the weights
>
> b) update BB->SuccBB with (1- Freq(Pred->BB)/Freq(BB->SuccBB))
>
> c) post-normalization: scale all weights such that the weight sum is 2^20
>
> Some helper routine can be defined.
>
>
>
>
This method looks good. I have two questions:
1. How to tell if the sum is too small? Can we just normalize all weights such that the sum of them is 2^20?
2. Is c) necessary?
http://reviews.llvm.org/D10979
More information about the llvm-commits
mailing list