[llvm-commits] [PATCH] treat calls to noreturn functions as side effecting

[Richard Osborne]

On 23/07/12 19:52, Nick Lewycky wrote:
> Duncan Sands wrote:
>> Hi Nuno,
>>
>> On 23/07/12 17:47, Nuno Lopes wrote:
>>> Quoting Duncan Sands<baldrick at free.fr>:
>>>
>>>> Hi Chandler,
>>>>
>>>>> Anyways, LLVM, much to my regret, decided to make infinite loops a magical
>>>>> observable side-effect, and not delete them. One of the consequences of this
>>>>> decision is that there is one of two bugs in the current system now:
>>>>>
>>>>> 1) functionattrs shouldn't mark functions with infinite loops as readonly, or
>>>>> 2) noreturn functions should have the same "observable" property as infinite
>>>>> loops.
>>>>>
>>>>> As there are many noreturn functions which do not have infinite loops and to
>>>>> which this magical property really shouldn't apply, I strongly prefer #1. It
>>>>> also clarifies the cost of this treatment of infinite loops -- they are
>>>>> implicitly a side-effect carrying construct.
>>>>>
>>>>> So in essence, I think this is a bug in functionattrs which stems from the
>>>>> decision to not delete infinite loops.
>>>> I disagree.  Read-only is what it says: the function only reads memory, doesn't
>>>> write it.  What has that to do with infinite loops?  I would rather see another
>>>> attribute, call it "finite", for functions that can be proved to complete in
>>>> finite time.  Then a function call with no uses can be deleted if read-only,
>>>> nounwind and finite.
>>> I agree with Duncan.
>>> Overloading readonly is not the way to go, since their usage is quite different.
>>> For example, alias analysis cares about pointers not escaping (and
>>> readonly/readnone are useful to prove that), but it doesn't care about
>>> non-terminating functions. BTW, readnone functions may not terminate either.
>>> That said, computing termination of loops is a really tricky business.
>> this is probably why no one implemented this :)
> But I have implemented this:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20100705/103998.html
>
> One of the truly unfortunate quirks in the implementation is that
> FunctionAttrs is a CGSCC pass and we need SCEV which is a FunctionPass,
> and that doesn't work. Chandler is working on fixing that. The patch in
> that link actually handles it by having a dumb ModulePass analysis that
> uses SCEV, and that FunctionAttrs uses. Lunacy.
>
> Chris was concerned that it we wouldn't be able to solve it much of the
> time, as seen in this paragraph:
>
>   > The other thing that concerns me is that there is a lot of cases that
>   > really can't be handled.  Iterating over an std::map or another
> node-based
>   > container can't be handled with this.  If you have some sort of front-end
>   > attributes (like the halting attribute you proposed for clang) or other
>   > language semantics that would be useful, then it makes sense.
>
> but I think that's something we have to live with. I eventually gave up
> on the thread because I wasn't able to easily find examples of things
> that were broken with the current status quo.
>
> Nick
Yes, I saw this thread - I didn't try and resurrect your patch because I 
suspected I'd run into the same problems you did (CGSCC pass needing a 
function pass). I wanted to do something simpler which, while not a 
complete fix to PR965 would still be an improvement on the current 
situation. If people don't think this isn't a useful step on the way 
then I think it would be best to leave this issue for now and revisit it 
after the proposed changes to the pass manager have landed.

-- 
Richard Osborne | XMOS
http://www.xmos.com