This is an archive of the discontinued LLVM Phabricator instance.

[attrs] Split off the forced attributes utility into its own pass that can be potentially run much earlier than FuncitonAttrs proper.
ClosedPublic

Authored by chandlerc on Dec 19 2015, 12:01 PM.

Details

Summary

This, for example, would allow forcing optnone or other widely impactful
attribute. It is also a bit simpler as the force attribute behavior needs no
specific iteration order.

An important question is whether it is useful to add this to the default pass
pipelines or not. I'm indifferent, happy to take suggestions. If so, it should
go at the very start so that optnone takes full effect, etc.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc updated this revision to Diff 43307.Dec 19 2015, 12:01 PM
chandlerc retitled this revision from to [attrs] Split off the forced attributes utility into its own pass that can be potentially run much earlier than FuncitonAttrs proper..
chandlerc updated this object.
chandlerc added a reviewer: jmolloy.
chandlerc added a subscriber: llvm-commits.
chandlerc updated this revision to Diff 43308.Dec 19 2015, 12:06 PM

Finish adding support for the forceattrs pass to the new pass manager and test it.

sanjoy added a subscriber: sanjoy.Dec 19 2015, 1:55 PM

Added some minor drive by comments inline.

include/llvm/Transforms/IPO/ForceFunctionAttrs.h
10 ↗(On Diff #43308)

The comments seem out of sync (possibly from StripDeadPrototypes)?

lib/Transforms/IPO/ForceFunctionAttrs.cpp
81 ↗(On Diff #43308)

Minor, but might be clearer as

if (!F.hasFnAttribute(Kind))
  F.addFnAttr(Kind);

(unless you plan to add more things to the non continue; branch later)

chandlerc added inline comments.Dec 19 2015, 1:56 PM
include/llvm/Transforms/IPO/ForceFunctionAttrs.h
10 ↗(On Diff #43308)

Yea, sorry, will fix them to be correct.

lib/Transforms/IPO/ForceFunctionAttrs.cpp
81 ↗(On Diff #43308)

Just moving code around here. I can clean it up in a follow-up patch?

majnemer added inline comments.
include/llvm/Transforms/IPO/ForceFunctionAttrs.h
1 ↗(On Diff #43308)

Shouldn't this be 80 col long to match?

lib/Transforms/IPO/ForceFunctionAttrs.cpp
15–20 ↗(On Diff #43308)

Please sort these.

sanjoy added inline comments.Dec 19 2015, 2:22 PM
lib/Transforms/IPO/ForceFunctionAttrs.cpp
81 ↗(On Diff #43308)

sgtm.

chandlerc updated this revision to Diff 43321.Dec 20 2015, 2:49 AM

Run clang-format, fix up comments.

chandlerc marked 7 inline comments as done.Dec 20 2015, 2:50 AM

All comments addressed.

One nit inline. Fixing it as a followup would be fine.

Also it makes me vaguely uncomfortable that the list of forceable attributes is buried in this piece of code. I guess it's for debugging/testing only, so something more formal isn't really warranted?

lib/Transforms/IPO/ForceFunctionAttrs.cpp
51 ↗(On Diff #43321)

I see it's a cut-and-paste but this one is out of order.

I didn't introduce this functionality, so I'm not particularly invested in its current design, but I'd like to save that discussion for later. I'd suggest something on llvm-dev and CC-ing the author. The question is, does this refactoring make sense given that the functionality is already in the tree...

lib/Transforms/IPO/ForceFunctionAttrs.cpp
51 ↗(On Diff #43321)

Yea, I'd like to leave cleanups of this for follow-up. I really just want to move this code first and make sure folks are comfortable with that.

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

Generally LGTM, but I *really* would like to see this in the default pipeline. Its primary purpose is to allow users to fiddle around with the heuristics a bit (alwaysinline/noinline are probably the most useful attributes we have), and given how negligible the performance impact of this is I'm very much for it being enabled by default so users didn't have to recompile their compiler to do some heuristic exploration.

Aside:

I'd suggest something on llvm-dev and CC-ing the author.

As the author, I can reply here :) I'm not particularly invested either in this parse function and didn't want to do it this way. What there ideally should be is some method on Attribute to parse the textual form of an enum attribute. That's not currently possible - you get a String attribute without an associated enum number.

The attribute parsing logic lives in the AsmParser, so extracting it out of there into Attribute would seem to me the right thing to do longer term. For this patch, it was simply for debugging / testing / exploration and the duplicated code was limited, so it didn't seem worth the refactoring investment.

I can however reconsider this if people think it's worth it :)

This revision is now accepted and ready to land.Dec 21 2015, 2:16 AM

Thanks James, I've added it to the pass manager builder in comparable places and I'm submitting. Feel free to move around if you like!

This revision was automatically updated to reflect the committed changes.
llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp