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

[Chandler Carruth via llvm-commits]

chandlerc marked 3 inline comments as done.
chandlerc added a comment.

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!


http://reviews.llvm.org/D15676