[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D85091#2252632 <https://reviews.llvm.org/D85091#2252632>, @Mordante wrote:

> In D85091#2250657 <https://reviews.llvm.org/D85091#2250657>, @rsmith wrote:
>
>> Looking specifically for attributes in the 'then' and 'else' cases of an `if` seems like a fine first pass at this, but I think this is the wrong way to lower these attributes in the longer term: we should have a uniform treatment of them that looks for the most recent prior branch and annotates it with weights. We could do that either by generating LLVM intrinsic calls that an LLVM pass lowers, or by having the frontend look backwards for the most recent branch and annotate it. I suppose it's instructive to consider a case such as:
>>
>>   void mainloop() noexcept; // probably terminates by calling `exit`
>>   void f() {
>>     mainloop();
>>     [[unlikely]];
>>     somethingelse();
>>   }
>>
>> ... where the `[[unlikely]];` should probably cause us to split the `somethingelse()` out into a separate basic block that we mark as cold, but should not cause `f()` itself or its call to `mainloop()` to be considered unlikely -- except in the case where `mainloop()` can be proven to always terminate, in which case the implication is that it's unlikely that `f()` is invoked. Cases like this probably need the LLVM intrinsic approach to model them properly.
>
> We indeed considered similar cases and wondered whether it was really intended to work this way. Since this approach seems a bit hard to explain to users, I changed the code back to only allow the attribute on the substatement of the `if` and `else`. For now I first want to implement the simple approach, which I expect will be the most common use case. Once finished we can see what the next steps will be.

+1 to the cautious approach. While I can understand how to implement what Richard is suggesting, I'm not convinced it results in readable code or that we're missing important optimization cases by using the more restrictive approach, so I'd like to get some field experience with users first.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85091/new/

https://reviews.llvm.org/D85091