[PATCH] D15676: [attrs] Extract the pure inference of function attributes into a standalone pass.

Mon Jan 4 04:48:58 PST 2016

Hi Chandler,

It's worth saying that I *did* look at all of the passes that introduce CG
edges and was convinced they didn't fall over with norecurse. I may have
been incorrect in my analysis however!

> Currently, combinations of mergefuncs and norecurse can cause miscompiles
of programs.

I've re-checked and I don't think this is a problem.

Firstly, mergefuncs will only merge functions with identical attributes. So
the only case we need to worry about is merging a norecurse function into a
norecurse function. Mergefuncs will also only merge functions with
identical outgoing CG edges.

Consider two merge candidates: A and B. Each of these is marked norecurse
and has a set of outgoing edges which must be identical as mentioned above.
In this case each function has just one outgoing edge, to 'w':

A -> w
B -> w

Control can never pass from w to A because A is marked as norecurse.
Control can never pass from w to B because B is marked as norecurse.

Therefore, the merged function A' (or B') will never recurse.

I have checked all other passes too and I don't think there's a problem.
Proofs have never been my strong suit though so please burst my bubble
gently.

Cheers,

James

On Mon, 28 Dec 2015 at 00:18 Chandler Carruth <chandlerc at gmail.com> wrote:

> I don't think there is a problem here with the release. Even if we decide
> we want an easier to deduce or less surprising attribute (or more
> attributes, all of these options have been discussed) it would be trivial
> to conservatively upgrade norecurse by dropping it.
>
> On Sun, Dec 27, 2015 at 9:59 AM Pete Cooper <peter_cooper at apple.com>
> wrote:
>
>> Is there going to be any issue with keeping norecurse in the short term,
>> cutting 3.8, then removing that attribute?
>>
>> That is, can we even remove an attribute or are we stuck with it forever
>> because we support old bitcode? Guess it depends on whether it can be auto
>> upgraded or not. Just worth a thought if it's worth trying to fix this
>> before a new branch is cut.
>>
>> Cheers
>> Pete
>>
>> Sent from my iPhone
>>
>> On Dec 22, 2015, at 7:03 PM, Philip Reames via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>> I agree w/James perspective here.  In particular, I feel like the issue
>> with norecurse is how we chose to specify it and not the notion itself or
>> the implementation thereof.  We should raise some of the issues on llvm-dev
>> and hammer through them, but I don't think we need to revert anything in
>> the meantime.  This doesn't feel like a major problem or the fundamentally
>> wrong approach.
>>
>> Philip
>>
>> On 12/21/2015 11:51 AM, James Molloy via llvm-commits wrote:
>>
>> Hey Chandler,
>>
>> I'll think this through and give a more reasoned reply in the morning.
>> However I would just say that I'm not opposed to an alternative
>> implementation that meets the same ends.
>>
>> I did put a lot of effort into this current solution though and it is
>> *significantly* less broken than what was in LLVM before (main is non
>> recursive, unconditionally!!). It currently gives nontrivial speed ups to
>> workloads we care about, so ripping it all out before designing something
>> new (likely taking at least a month given the season) isn't massively
>> appealing.
>>
>> Cheers,
>>
>> James
>> On Mon, 21 Dec 2015 at 19:40, Chandler Carruth <chandlerc at gmail.com>
>> wrote:
>>
>>> I'm getting the strong indication that norecurse isn't actually a good
>>> idea, and hasn't been hammered through enough.
>>>
>>> It turns out that I also need to build brand new infrastructure to keep
>>> supporting it.
>>>
>>> I'm ripping it out of this patch clearly, but I am wondering whether we
>>> want to rip it out entirely and start a new discussion on the actual
>>> problem James is trying to solve and the best way to go about it.
>>>
>>> Note that, for example, if strlen and memcpy can't be marked as
>>> norecurse (and I agree that they can't in a strict sense as it is
>>> conforming to implement both of them as recursive algorithms) then *any*
>>> inference of norecurse in LLVM is actually broken today because we may add
>>> calls to non-norecurse functions (namely memcpy) at a later point. Given
>>> that, I think the spec for norecurse is too narrow to be a terribly useful
>>> property. I suspect that James will need to use some other strategy to get
>>> the useful optimizations he's after (which center around *main* not being
>>> recursive, a very different beast).
>>>
>>> -Chandler
>>>
>>> On Mon, Dec 21, 2015 at 10:09 AM Mehdi Amini <mehdi.amini at apple.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> Sent from my iPhone
>>>>
>>>> On Dec 21, 2015, at 12:07 AM, James Molloy < <james at jamesmolloy.co.uk>
>>>> james at jamesmolloy.co.uk> wrote:
>>>>
>>>> Hi Mehdi,
>>>>
>>>> > For strlen: you’re right that it is not correct in the absolute, but
>>>> for what we use the information I’m not sure it matters. We’re interested
>>>> in the fact that it won’t call back in the user code in any way, so that we
>>>> can infer the norecurse on the callers.
>>>>
>>>> I understand where you're coming from, but I think second-guessing how
>>>> an attribute may be used is a dangerous route to go down. "norecurse" could
>>>> be used for something else in the future, so unless we have a good reason
>>>> it would be good not to make assumptions.
>>>>
>>>>
>>>> Agree, but if we go on the "safe way" now it means we should either
>>>> redefine the norecurse attribute (like the almostreadnone for example) or
>>>> have another one.
>>>>
>>>> Mehdi
>>>>
>>>>
>>>>
>>>> James
>>>>
>>>> On Mon, 21 Dec 2015 at 10:06 James Molloy via llvm-commits <
>>>> <llvm-commits at lists.llvm.org>llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> jmolloy accepted this revision.
>>>>> jmolloy added a comment.
>>>>> This revision is now accepted and ready to land.
>>>>>
>>>>> Hi Chandler,
>>>>>
>>>>> Thanks for taking the time to do this.
>>>>>
>>>>> I agree with Philip - while I like that you've added extra norecurse
>>>>> attributes, I don't like that it's smuggled in as part of this patch. I'd
>>>>> highly prefer this patch to be NFC and then a simple update patch adding
>>>>> the norecurse attributes.
>>>>>
>>>>> Apart from that and the specific comment below, it LGTM.
>>>>>
>>>>> Cheers,
>>>>>
>>>>> James
>>>>>
>>>>>
>>>>> ================
>>>>> Comment at: include/llvm/Transforms/IPO/InferFunctionAttrs.h:25
>>>>> @@ +24,3 @@
>>>>> +/// A pass which infers function attributes from the names and
>>>>> signatures of
>>>>> +/// function declarations in a module..
>>>>> +class InferFunctionAttrsPass {
>>>>> ----------------
>>>>> Double period.
>>>>>
>>>>> ================
>>>>> Comment at: include/llvm/Transforms/IPO/InferFunctionAttrs.h:26
>>>>> @@ +25,3 @@
>>>>> +/// function declarations in a module..
>>>>> +class InferFunctionAttrsPass {
>>>>> +public:
>>>>> ----------------
>>>>> Unrelated: Damn, new style passes look so much nicer!
>>>>>
>>>>>
>>>>> http://reviews.llvm.org/D15676
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>
>>
>> _______________________________________________
>> llvm-commits mailing listllvm-commits at lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160104/79ec640f/attachment.html>