tejohnson added a comment. In D132186#3750925 <https://reviews.llvm.org/D132186#3750925>, @iamarchit123 wrote:
> @paulkirth this change was done under the intuition that marking hot > functions noinline may hurt performance. This change till now hasn't been > tested for performance improvements and so the points that you raise that > functions marked noinline are marked not for hotness/performance but rather > correctness is something that I was unaware of. Since I don't have access to > big services currently and can't test this change, so I may be unable to > defend this change. @modimo feel free if you think folks can come back and > test this change for effectiveness or completely redo this if the above > change is still insufficient. > > I agree until it's shown/proven that there is a serious performance win this > change may not be useful. Thanks for the review. I have seen a few cases where noinline was used for performance, in addition to other cases like avoiding too much stack growth. I've also seen it used without any comment whatsoever. So I think it would be good to make it easier to identify cases where we are blocked from inlining at hot callsites because of the attribute. It is a little different than misexpect though in that the expect hints are pretty much only for performance, so it is more useful to be able to issue a strong warning that can be turned into an error if they are wrong. And also there was no way to report the misuse of expects earlier, unlike inlining where we already had the remarks plumbing. I haven't looked through the patch in detail, but is it possible to use your changes to emit a better missed opt remark from the inliner for these cases (I assume we will already emit a -Rpass-missed=inline for the noinline attribute case, just not highlighting that it is hot and would have been inlined for performance reasons otherwise)? I suppose one main reason for adding a warning is that the missed inline remarks can be really noisy and not really useful to the user vs a compiler optimization engineer doing inliner/compiler tuning, and therefore a warning would make it easier to turn on more widely as user feedback that can/should be addressed in user code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132186/new/ https://reviews.llvm.org/D132186 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
