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

Tue Jan 5 14:39:45 PST 2016

----- Original Message -----
> From: "Chandler Carruth via llvm-commits" <llvm-commits at lists.llvm.org>
> To: "Philip Reames" <listmail at philipreames.com>, "James Molloy" <james at jamesmolloy.co.uk>, "Mehdi Amini"
> <mehdi.amini at apple.com>
> Cc: reviews+D15676+public+9cd4fa61f763fcf4 at reviews.llvm.org, llvm-commits at lists.llvm.org
> Sent: Tuesday, December 22, 2015 1:33:49 PM
> Subject: Re: [PATCH] D15676: [attrs] Extract the pure inference of function attributes into a standalone pass.
> 
> 
> 
> 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. =/
> 

It seems like we're discussing two separate topics here:

 1. norecurse - This matters for a function we're actually compiling, and says that nothing we call, no matter how marked, can cause us to recursively reenter the function.

 2. no_calls_to_defined_functions (module bikeshed of course) - Indicates a function that does not, either directly or indirectly, call any function for which we can currently see the definition.

(2) can help to infer (1), but it is a strictly weaker concept.

 -Hal

> 
> 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 list llvm-commits at lists.llvm.org
> http://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
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory