[PATCH] D36850: [ThinLTO] Add norecurse function attribute propagation

[Di Mo via Phabricator via cfe-commits]

modimo added a comment.

In D36850#2990907 <https://reviews.llvm.org/D36850#2990907>, @tejohnson wrote:

> In D36850#2990847 <https://reviews.llvm.org/D36850#2990847>, @modimo wrote:
>
>> In D36850#2990771 <https://reviews.llvm.org/D36850#2990771>, @tejohnson wrote:
>>
>>> In D36850#2968536 <https://reviews.llvm.org/D36850#2968536>, @modimo wrote:
>>>
>>>> In D36850#2964293 <https://reviews.llvm.org/D36850#2964293>, @tejohnson wrote:
>>>>
>>>>> Good point on indirect calls. Rather than add a bit to the summary, can the flags just be set conservatively in any function containing an indirect call when we build the summaries initially? I think that would get the same effect.
>>>>
>>>> That could have an issue where A calls {indirect, B} and A gets propagated upon from B without knowledge that the indirect call exists. Right now I've got a FunFlags `hasUnknownCall` which marks these as non-propagatable.
>>>
>>> Ah, because there isn't a conservative setting of the flag...which raises a larger issue (but maybe I am completely missing something) - how do we distinguish between the NoUnwind summary flag not being set because we don't know yet (in which case we want the propagation from callees), vs because it cannot be NoUnwind (because there is a throw in the function)? Do we need to have a second flag indicating that a function contains a mayThrow instruction (other than calls, which are being handled by the propagation)?
>>
>> Only call instructions can throw (what ultimately performs the throw operation is an opaque call to __cxa_throw()) which simplifies the problem. If all calls are known, we only need to examine the callees for accurate propagation.
>
> What about the other instruction checks done in Instruction::mayThrow i.e. http://llvm-cs.pcc.me.uk/lib/IR/Instruction.cpp#592?

Good point! CleanupReturnInst and CatchSwitchInst are windows SEH specific representations for asynchronous exceptions but definitely should be covered for correctness. For ResumeInst it's the "return" of a landing pad and in order for a landing pad to be reachable AFAIK an invoke must exist so is captured by the call graph. I'll add a scan for `Instruction::mayThrow` in summary building. Having a mayThrow flag or making NoUnwind a tri-state flag in the summary makes sense to me to capture this case.

As a side note to why there's a check for ResumeInst at all: an invoke instructions actually never has "mayThrow" set. I haven't delved too deep but this could be changed since a dead landing pad at attribute inference time can lead to pessimization of NoUnwind in cases I've seen (alternatively, making sure CFG opts run before this to make sure this is pruned away).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36850