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

Chandler Carruth via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 22 11:33:49 PST 2015


After a long discussion with James, I mostly think this is the right
approach, but it is pretty badly incomplete.

Currently, combinations of mergefuncs and norecurse can cause miscompiles
of programs. We need to really carefully audit the few passes that actually
introduce call edges to fix all of the ways they can introduce recursion.
It's a bit alarming that all of this infrastructure is on and being used by
globalopt when that audit hasn't taken place, but I trust James will get to
it (real soon i hope?)

We also need *something* to annotate strlen and other things with,
otherwise it becomes obnoxiously hard to actually infer norecurse anywhere
and we're burning *considerable* infrastructure and compile time trying to
do so. It's like noreentrant but not quite that exact concept. =/ I dunno
what to call it. But unless we can reasonably mark memcpy and strlen and
friends as not calling back out to user code, we're in the awful place of
doing lots of work to try to infer norecurse but failing for every single
function that does a struct copy or uses std::string. =/

Anyways, I'll rip norecurse out of this patch clearly, as it implies far
more than I thought. I'm also struggling to find a way to actually
represent the inference we have in a clean way (the finalization trick
is... not going to work).

-Chandler

On Tue, Dec 22, 2015 at 11:04 AM Philip Reames <listmail at philipreames.com>
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>
>>> 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> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151222/d9fac02b/attachment-0001.html>


More information about the llvm-commits mailing list