- User Since
- Mar 13 2016, 1:10 PM (74 w, 6 d)
Sep 20 2016
Sep 19 2016
Thank you for the positive feedback and the discussion! I updated the above summary to reflect the insight that Johannes provided.
Sep 14 2016
Aug 3 2016
Thank you for committing!
I now rebased the patch (I thereby also replaced my introduced cast<>s by dyn_cast<>s which is cleaner now, sorry for this post-acceptance change). I also reran make check and lnt now and the tests pass - all but the MultiSource/Benchmarks/MiBench/consumer-typeset/consumer-typeset.execution_time in lnt, but this seems to be independent from my changes since it also fails when testing with a clean llvm build that doesn't include my patch (I ran lnt via myvirtualenv/bin/lnt runtest nt --sandbox /tmp --cc ~/opt/llvm_release+asserts/bin/clang --test-suite ~/repos/llvm-test-suite). I don't know if that's a known issue, but at least I could not find an according bugzilla entry - I'll try to further investigate this.
Aug 2 2016
Cool, thank you for reviewing! Since I don't have commit privileges, somebody else may have to merge this. Should I therefore rebase and reupload the patch?
Aug 1 2016
Hi Sanjay, thank you for jumping in and for adding subscribers!
Jul 27 2016
Jul 26 2016
Jul 20 2016
Thank you for committing. See https://reviews.llvm.org/D22561 for additional test cases.
Jul 19 2016
Thank you, I updated the test cases accordingly.
Jul 18 2016
Thank you for bringing this to my notice @majnemer.
Thank you for the feedback, I updated the patch accordingly.
Fixed some typos.
I updated the patch now to only contain style changes (see title and summary for detailed information). And you were right Tobias (regarding \brief), since autobrief is enabled \brief can safely be dropped here.
Thank you a lot for this extensive feedback, Tobias! I agree that it makes to make this an NFC for the style changes for now. As soon as this is merged I will open a separate PR for the functional changes then. Before updating the patch I also have a few questions (also see my responses to your inline comments).
Jul 17 2016
Sorry, I should have mentioned that I also don't have commit access on the repo. Thank you for committing @gareevroman.
I'll close this again, since I forgot to initially subscribe llvm-commits to this. Sorry for the inconvenience.
Is there any additional action required from my side in order to merge this change?
Thank you for all the comments. I close this now, in favor of a different approach. The recent discussion on this can also be followed at https://groups.google.com/forum/#!topic/julia-dev/5io1KnEswqs
Jun 27 2016
Thank you for your comments Michael! However, regarding Sanjoy's concerns, I am unsure if it's really possible to change these patterns to be applicable in cases where the absence of a wraparound cannot be assured, like in the provided test cases. I hope not to overlook obvious opportunities for such a generalization. However, if it's indeed unattainable at the level of ScalarEvolution, this would at least unveil the necessity to handle this via canonicalization on the side of Julia's LLVM code generator. If true, I am sorry that this patch was needed to gain this insight.
Jun 25 2016
Thank you for bringing this to my notice, I will address these issues and update the patch.
Unfortunately, these changes cause the function createNodeForSelectOrPHI to get quite long. Originally I tried to combine the added code with the existing pattern matching logic in the ICMP_SGE and ICMP_UGE case branches. But in my opinion this caused the code to get too unclear, so I decided to separate the logic. In case the provided test cases are too compact, I can also try to define more extensive ones. Also I was not sure where the existing pattern matching logic is tested, therefore, for now, I added a separate test file for this.
Jun 8 2016
Jun 5 2016
May 22 2016
I'm sorry, I was away this weekend. I applied a bigger style for the headings now.