- User Since
- Apr 13 2015, 9:48 AM (227 w, 4 d)
LLC miss info should be very sparse, so mixing it with LBR may not bring a lot of benefit. Having mixure of different types of profile data can lead to combinatorial growth of the number of readers. As the number of profile types increases, it can cause the reader to be unmaintaintable. That is why I suggest making it clean -- one type per section.
Wed, Aug 21
llvm-profdata should also have a round trip test:
Mon, Aug 19
Can you split the format extension change into a separate patch?
Fri, Aug 9
static prediction is intended to help common use cases, so for corner cases when NaNs are used frequently, user will need to resort to source annotation (such as builtin_expect).
Thu, Aug 8
Wed, Aug 7
See Some minor name suggestions, also
Mon, Jul 29
This version looks fine to me, but please let other reviewers to weigh in as well.
Thu, Jul 25
I suspect other affected cases are due to bad static profile data too.
Wed, Jul 24
Jul 24 2019
I did some test with the patch with samplePGO and it works fine. LGTM
It is also better to introduce a coldness factor parameter 'F', i.e, if the source block's count is less than 1/F of the header count, suppress the hoisting.
Jul 22 2019
Hal probably remembered more details. IIRC, the argument was that LICM provides IR canonicalization which can simplify analysis -- there were some examples about exposed vectorization opportunities etc.
This was tried before. IIRC, the conclusion was to implement look sinking pass to undo non-profitable LICM. The loop sinking pass was in D22778. Can the loop sinking pass be enhanced to handle the case here ( I have not looked in details) ?
Jul 19 2019
carrot, can you help looking into this? The cost model should look into extra direct jumps introduced as well.
Jul 18 2019
Is the test case slow down with PGO or not? Also do you have branch misprediction perf data? (large slowdowns like this is usually triggered by side effect like this). Is there a trimmed down version to demonstrate the issue?
Jul 16 2019
Jul 12 2019
Jul 11 2019
Can you put an example (text CFG) showing cases where tailDup is bad -- possibly as a source comment?
Jul 10 2019
Jul 9 2019
Some end-to-end tests as in compiler-rt are probably helpful here.
Is it possible to split the patch into a NFC refactoring and one functional change?
Jul 8 2019
Jul 5 2019
Jul 4 2019
please flip the flag to false by default and check in the rest first.
Jul 3 2019
my test hit some other unrelated debug assert. Can you first change the debug assert into error message so that there is no need to use assert enabled build to test the option?
Does the bursty style sampling provide additional benefit? A non-bursty style sampling can be cheaper:
Do you have a functional test case to show what this patch is trying to accomplish/fix?
Jul 2 2019
let me do some internal PGO build tomorrow with the option on and get back.
What types of testing has been done ? I assume the code base now is relatively clean so it is safe to turn this on by default?
Jul 1 2019
Jun 28 2019
Rong, can you take a look at this patch?
Jun 24 2019
The refactoring part looks ok. +vsk to double check.
Jun 21 2019
Is there a bug reference somewhere?
Jun 13 2019
llvm-cov's user manual probably also needs some update.
is thin archive supported?
Jun 11 2019
Jun 10 2019
This can be handled this way:
Warning. With static profiling, the layout strategy based on the 'precise' cost model may be off. If for some reason this causes issues later, the change should be guarded with 'hasProfile' check.
Jun 7 2019
Jun 6 2019
Jun 5 2019
It is just more convenient for you to contribute to LLVM in the future. I can help you commit this time.
I suggest you apply for commit right first. There are likely fallouts that need to be dealt with.
lgtm. Watch out for problems on other platforms.
Jun 4 2019
Jun 3 2019
lgtm assuming the state tracking code gets removed (or put under NDEBUG in some way) after the cleanup.
May 31 2019
For all the test cases update, please also validate if they make sense or not if possible.
once the cleanup is done, the state tracking should be removed as well.
May 30 2019
I suggest the following path:
- add an option to enforce internal error on inconsistency; by default the inconsistency is ignored.
- fix the exposed root cause one by one
- turn on the option by default
Sounds reasonable -- the intention is clear with this -- the interface tells runtime to do whatever needed to ensure merging. Please send a patch with merging test case.
What I meant is that we should never see inconsistent state and instead of setting invalid state, we assert there. Ideally, the profile setting/update APIs should help user avoid creating the inconsistent state in the first place (i.e., catch the problem on the spot).
May 29 2019
I think it is cleaner for user to make the intention clear by passing the flag if the file handle is passed to the runtime. Otherwise the runtime will have to guess/assume user's intention (e.g, also reopen the file with "r+b" mode for merging).
To support merging, the new interface needs to take more
May 28 2019
Why is symbol remapping an issue? old MD5 --> input symbol name -> output symbol name --> mapped MD5sum ?
what is the main blocker for the longer term solution?
Longer term, the plan is to put a white list of the symbols into the profile data so that the compiler can decide if a function is newly created or simply cold.
There should be a test case.
May 27 2019
May 24 2019
Does this patch handle the case in D62079? If yes, you can move the tests added in that patch here.
May 23 2019
May 22 2019
Indeed. The original one is O(N^2). LGTM.
was there a quadratic behavior before ? It seems linear before and after the patch -- but just cut the computation by a factor of 2 . Do you need an option to control the max chain length?
May 21 2019
Loop rotation not only affects fall through placement of incoming and exit edges, but internal fall through as well -- it converts the original backedge into a new fallthrough while converts an original internal fallthrough into the backedge. In other words, the overall cost/benefit needs to consider all changes. Doing pattern matching like this can lead to wrong decisions.
May 20 2019
I like the direction this patch is going, but I think it can do better.
May 15 2019
May 14 2019
May 10 2019
it is probably better to put the tests in the same file with different check prefixes.