paulkirth added a comment. To be clear, I'm not morally opposed to your patch, I just wanted to understand the context more completely and why this is the best approach. And like I said, I can't recall encountering a place where `noinline` was done for performance reasons. Code size and correctness are the two reasons I've seen commonly though.
In D132186#3750450 <https://reviews.llvm.org/D132186#3750450>, @iamarchit123 wrote: > I was under the impression that it was used for performance reasons and hot > functions should not ideally be marked noinline. @modimo could you also pitch > in on this? If not function entry count what other things could possibly > indicate a false usage of noinline(if not the hotness) Using the entry count isn't the issue, its that people don't normally mark their hot code `noinline`, certainly not w/o a reason. I'm open to the idea that there might be a performance reason to do so, but I'm aware of other reasons that are more common in my experience. While you've gotten good use from the diagnostic, I'm unsure how well that generalizes. > This can be a separate pass but even I am using Cost Analysis to check > noinline functions cost apart from function entry count, so it felt natural > to integrate the noinline warning into inline cost analysis. If the goal is to disclose that the annotation may be //wrong//, then shouldn't you want to report that regardless of the cost analysis? The inliner's decision is orthogonal to whether the attribute is beneficial or potentially incorrect, right? > I think there is no Optimization remark emission for missed inlining > opportunities for noinline functions as cost analysis is skipped for > functions marked with noinline attribute. Right, but your new diagnostic seems to be exactly like those remarks, which is why I brought them up. You //are// issuing a diagnostic regarding a missed inlining opportunity afterall, so I don't think its strange to suggest that you should consider reporting these through the same mechanism. If there's a good reason not to, that's fine, but then I would expect there to be some rationale. > No we don't want any builds to fail with this. Our only aim to make this > change is to emit a warning for functions which a user may have accidentally > marked noinline but removing noinline attribute may give some performance > benefits. Then using a remark seems to be the better choice. There are lot's of places that compile w/ `-Werror`, and making this a warning ensures builds fail. That was a goal for Fuchsia w/ MisExpect, so if it's not in this case, you may want to consider only using remarks as an alternative, since they are always only informational. Like I said before, I'm not opposed to this, but I'd like to understand why the current infrastructure and diagnostics are insufficient or shouldn't just be updated to also report this case. My other concern is that there are significantly more cases where `noinline` is used for correctness than there are for performance reasons, which may dilute the usefulness of the diagnostic. 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
