- User Since
- Oct 6 2015, 11:37 AM (281 w, 1 d)
Oct 16 2017
Oct 12 2017
In general, please follow existing style for the file you're editing.
Oct 6 2017
Oct 5 2017
Oct 4 2017
If this got dropped I'm sorry.
Sep 11 2017
I'm OK with this.
Sep 8 2017
This looks fine overall to me.
Sep 5 2017
I don't think this should go in, even off by default as long as it's broken. You can pull the produceSameValue check or fix it, but I don't like having a known-broken pass in tree.
Aug 31 2017
Aug 30 2017
Aug 24 2017
dannyb asked me to take over the review.
Can you fix the issues that filcab identified? I'll see if I can get you an answer about critical edge splitting.
Aug 16 2017
Do you plan on doing something similar for the DSB decode cache issues that occasionally arise?
Specifically this: https://bugs.llvm.org/show_bug.cgi?id=5615
Aug 4 2017
Should we adjust the ratio if it's not specified explicitly and no profile data is available?
Submitted in r310129
Aug 3 2017
needs a test case, and needs an adjustment to the frequency for non-profile functions.
Can you provide a description of what you had to change relative to the rollback, and how you're verifying that the issue that caused the rollback has been fixed?
Jul 10 2017
Looks fine. I would prefer the fallthrough checks be successor checks, but I can accept it either way.
Jun 30 2017
All the checks use unreachable now.
Jun 27 2017
Jun 26 2017
Use cast instead of dyn_cast
Jun 23 2017
Approved with the changes I've marked.
It would be nice to get rid of that negative completely. Not in this patch, but just remove it completely wherever we create CFI Instructions.
Jun 22 2017
This is looking really good. Thanks.
Jun 21 2017
Chandler, After r305934 A test for this isn't possible. We should still commit the fix.
Jun 19 2017
OK. It's looking pretty good overall. I looked a lot closer at the actual CFI Code. Mostly it looks great. I don't think you need AdjustCFAOffset at all. You spend a lot of work maintaining it, but in the one case that it's used, it's just (OutgoingOffset - IncomingOffset). It's only used with blocks that don't contain an offset def.
Mostly looks good. It will be easier to follow when you factor out the NFC sections and rebase.
Jun 15 2017
This is looking pretty good. I'm going to go back over, but most of my initial concerns have been satisfied.
Jun 8 2017
No longer needed because of https://reviews.llvm.org/D34017
Jun 7 2017
It doesn't have to blow it up exponentially. Can we clone the function to be inlined, re-writing the recursive calls to call the clone, and then inline that? Similar to a worker-wrapper transformation
This is coming along nicely. I forgot to say last time that I was pleased overall.
Jun 5 2017
Jun 2 2017
Some initial thoughts. I would like to hide the actual CFI algorithms from the existing passes as much as possible.
May 30 2017
Tidy up comparisons.
May 26 2017
Add comments to the change summary about the global heuristic that we're approximating (maybe badly) by calculating the branch factor.
This looks fine to me.
I think you counted wrong. The dynamic taken branch count is the same. But the distribution of the taken branch count is more consistent.
May 25 2017
I'm fairly certain these are right, but I wanted to get another set of eyes on them before I committed them.
May 17 2017
May 15 2017
Committed in rL303084
May 12 2017
If either threshold is the only one explicitly set, use that threshold.
Otherwise, if both, or neither are set, use the aggressive threshold at O3
No, I wanted to get agreement before I re-wrote it. I can do it if you'd like to see it before deciding.
May 11 2017
Looks fine with the comments I gave addressed.
Now with more Test Case!
Feel free to send me patches that update the other parts of IfConversion
May 10 2017
Add splat to the output.
May 9 2017
Ping? Do we know if this vector operation requires the byte splat on power9?
Looks good to me with the changes mentioned.
May 5 2017
Change vendor to unknown
May 4 2017
May 2 2017
Made the aggressive threshold an option.
Note that we should eventually produce the power9 sequence for the non-vector case as well.
Update test to check for the power9 sequence.