Page MenuHomePhabricator

[FunctionAttrs] Infer WriteOnly Function Attribute
ClosedPublic

Authored by homerdin on Jun 20 2018, 12:16 PM.

Details

Summary

These changes expand the FunctionAttr logic in order to mark functions as WriteOnly when appropriate. This is done through an additional bool variable and extended logic. Any feedback would be appreciated!

Diff Detail

Repository
rL LLVM

Event Timeline

homerdin created this revision.Jun 20 2018, 12:16 PM

This patch looks fine to me.

I inlined some minor comments. Also, you should add a dedicated test case or explain in the commit message why that isn't possible yet.

lib/Transforms/IPO/FunctionAttrs.cpp
177 ↗(On Diff #152130)

[Style] I'm not a fan of comment lines in a conditional without braces but I can see that you only copied the style already used here.

208 ↗(On Diff #152130)

[Typo] Two dots.

276 ↗(On Diff #152130)

Maybe you want to place an assertion here (or prior to the loop) that ensures ReadsMemory and WritesMemory are never both set. After this point, it is implicitly assumed anyway (Writes && Reads would otherwise lead to a ReadOnly attribute). Another benefit of an explicit assumption would be that it allows you to simplify the two conditions later on.

283 ↗(On Diff #152130)

[Typo] Leading space

homerdin updated this revision to Diff 157359.Jul 25 2018, 2:23 PM

Fixed typos and added assertion

Ping. Are you going to upstream this?

hfinkel accepted this revision.Aug 13 2018, 2:58 PM

A couple of minor comments, but otherwise, LGTM.

lib/Transforms/IPO/FunctionAttrs.cpp
209 ↗(On Diff #157359)

I'd write this like the line below:

WritesMemory |= I->mayWriteToMemory();
286 ↗(On Diff #157359)

Add space after if.

This revision is now accepted and ready to land.Aug 13 2018, 2:58 PM
This revision was automatically updated to reflect the committed changes.