Page MenuHomePhabricator

[attrs] Extract the pure inference of function attributes into a standalone pass.
ClosedPublic

Authored by chandlerc on Dec 20 2015, 2:53 AM.

Details

Summary

There is no call graph or even interesting analysis for this part of
function attributes -- it is literally inferring attributes based on the
target library identification. As such, we can do it using a much
simpler module pass that just walks the declarations. This can also
happen much earlier in the pass pipeline which has benefits for any
number of other passes.

In the process, I've had to clean up several aspects of this logic.
First, the pass now counts inferred attributes independently rather than
just counting all the inferred attributes as one, and the counts are
more clearly explained. Second, the pass infers at least some of the
library functions as norecurse. I've been *exceedingly* conservative
here and only added it for library functions that I could instantly and
confidently make this claim. Things like printf and malloc or any
function which I could imagine some how, even through very strange steps
such as malloc hooks or a recursive printf implementation, I left alone.
This at least allowed me to keep the no-recurse logic in place rather
than having to delete it for being unused just to add it back when we
decide to finish annotating such things. We should go through and be
more aggressive about norecurse I suspect.

The two test cases we had for this code path are both ... woefully
inadequate and copies of each other. I've kept the superset test and
updated it. We need more testing here, but I had to pick somewhere to
stop fixing everything broken I saw here.

Depends on D15668.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc updated this revision to Diff 43322.Dec 20 2015, 2:53 AM
chandlerc retitled this revision from to [attrs] Extract the pure inference of function attributes into a standalone pass..
chandlerc updated this object.
chandlerc added reviewers: jmolloy, mehdi_amini.
chandlerc added a subscriber: llvm-commits.
chandlerc updated this revision to Diff 43324.Dec 20 2015, 3:13 AM

Finish wiring up the new pass manager for inferring function attrs, and test it.

reames added a subscriber: reames.Dec 20 2015, 6:06 PM

For some reason, I can't seem to log in to phabricator at the moment...
A couple of comments below.

"InferFunctionAttrs" - bikeshed wise, this seems like a very generic,
non-discriptive name. Possibly: InferFunctionAttrsFromTLI? Or
something which otherwise gives a clue as to what we're infering from?

This really seems like it should somehow be a tablegen file. I know
you're just moving code around, but the long list of hand written code
for each function smells. Particularly the type checking of arguments
after TLI already matched to an enum...

Stale comment in "inferAllPrototypeAttributes".

I'm uncomfortable with the addition of the various no-recurse
attributes. I'm not saying your choices are wrong, but mixing them in
with a large code motion patch makes them hard to review. There's also a
question of what we're assuming about the runtime environment. After
all, there is a perfectly legal (if unlikely) recursive implementation
of strlen.

Otherwise, LGTM.

Philip

jmolloy accepted this revision.Dec 21 2015, 2:06 AM
jmolloy edited edge metadata.

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

include/llvm/Transforms/IPO/InferFunctionAttrs.h
25 ↗(On Diff #43324)

Double period.

26 ↗(On Diff #43324)

Unrelated: Damn, new style passes look so much nicer!

This revision is now accepted and ready to land.Dec 21 2015, 2:06 AM
probinson added inline comments.
lib/Transforms/IPO/InferFunctionAttrs.cpp
926 ↗(On Diff #43324)

s/everything/anything/

chandlerc marked 3 inline comments as done.Dec 27 2015, 12:43 AM

Updated to address all comments. I've stripped out the norecurse stuff. This means that we'll have to add back infrastructure for counting statistics on such attributes, which was the only reason I added it here. I tried to explain that clearly in the description of the change; there was no attempt to smuggle anything here.

Anyways, after long discussions about this attribute, its semantics are much more surprising than I originally realized (at least to me) and so clearly it shouldn't be inferred just yet. This is now *just* moving to an earlier inference model in a separate pass with more detailed statistics tracking.

To address Philip's points:

First, I've fixed the stale comment, thanks!

I do agree that this huge switch is... undesirable. But I'd like to save fixing this more deeply for some other day / time / person / project. There is more wrong than just the use of a huge switch. For example, Why does this only fire on declarations??? It should fire on definitions as well! We have the no-builtin mechanism that should prevent TLI from recognizing non-builtin routines that we shouldn't make such inferences about. Anyways, I'm very happy if someone wants to keep cleaning this up, but I'm going to prioritize the pass manager centric work.

Finally, I tried to think of a better name, but I kept coming back to this one. It really is a generic attribute *inference* pass. It doesn't analyze or compute anything, it just infers expected attributes based on a-priori information. At the moment that is encapsulated by TLI, but I don't think the pass should be restricted to the *source* of inference but merely to it being inference as opposed to logical analysis of the program in some way.

That said, I am very happy to change the name of this if you or others come up with something better, and I don't think that should block progress here, so I'm going to keep rolling and we can do a rename at-will.

Thanks all!

This revision was automatically updated to reflect the committed changes.