Page MenuHomePhabricator

[CodeExtractor] Use subset of function attributes for extracted function.
ClosedPublic

Authored by fhahn on Dec 17 2017, 3:53 PM.

Details

Summary

In addition to target-dependent attributes, we can also preserve a
white-listed subset of target independent function attributes. The white-list
excludes problematic attributes, most prominently:

  • attributes related to memory accesses, as alloca instructions could be moved in/out of the extracted block
  • control-flow dependent attributes, like no_return or thunk, as the relerelevant instructions might or might not get extracted.

Thanks @efriedma and @aemerson for providing a set of attributes that cannot be
propagated.

Event Timeline

fhahn created this revision.Dec 17 2017, 3:53 PM
fhahn added a comment.Jan 2 2018, 9:08 AM

Post holiday ping. Happy new year! This updates CodeExtractor to propagate the function attributes for the original function to the extracted function (and the call to the extracted function). As the extracted function is a subset of the original function, I think that should be safe and desired (e.g. correctly preserving fastmath flags or ARM thumbness)

Post holiday ping. Happy new year! This updates CodeExtractor to propagate the function attributes for the original function to the extracted function (and the call to the extracted function). As the extracted function is a subset of the original function, I think that should be safe and desired (e.g. correctly preserving fastmath flags or ARM thumbness)

What about the noreturn attribute? Obviously the extracted function must include a return so it seems this would need to be dropped.

fhahn added a comment.Jan 2 2018, 1:26 PM

What about the noreturn attribute? Obviously the extracted function must include a return so it seems this would need to be dropped.

Right, the part that does not return may not get extracted and would be executed after the call to the extracted function. So we should drop it, thanks! Are there any additional attributes we should be dropping?

fhahn added a comment.Jan 2 2018, 1:31 PM

I suppose we should also drop thunk, as the tailcall could stay in the non-extracted part and the extracted part may not end in a tailcall?

You also can't copy readnone/readonly/writeonly/argmemonly/inaccessiblememonly/inaccessiblemem_or_argmemonly: the extracted code could read/modify an alloca allocated in the parent function.

allocsize also can't be copied for obvious reasons. Same for returns_twice.

patchable-function doesn't make sense to copy; not sure it would do any harm, but doesn't do anything useful. Same with alignstack.

Not sure about convergent.

And of course, there's a whole bunch of target-specific attributes, but hopefully those are all safe to copy.

fhahn updated this revision to Diff 128614.Jan 4 2018, 8:46 AM
fhahn retitled this revision from [CodeExtractor] Use function attributes from original function. to [CodeExtractor] Use subset of function attributes for extracted function..
fhahn edited the summary of this revision. (Show Details)

Thanks Eli & Amara. I've substantially changed the code to use a subset of whitelisted function attributes from the original function for the extracted function.

efriedma added inline comments.Jan 4 2018, 12:07 PM
lib/Transforms/Utils/CodeExtractor.cpp
636 ↗(On Diff #128724)

Could we explicitly list out all the attributes here, so the compiler will warn if a new attribute gets added?

644 ↗(On Diff #128724)

Not sure about propagating JumpTable and Naked. (Granted, we probably shouldn't be extracting code from Naked functions anyway.)

fhahn updated this revision to Diff 128724.Jan 5 2018, 2:56 AM
fhahn marked 2 inline comments as done.
fhahn added a reviewer: silvas.

Explicitly state unsafe attributes and don't propagate Naked and JumpTable. Also adding Sean, as he added the target-dependent propagation I think.

rriddle added a subscriber: rriddle.EditedJan 5 2018, 6:52 AM

I was actually responsible for the target dependent attrs patch, Sean was nice enough to commit for me at the time.

This looks fine.

Would have looked sooner but I've been in Japan for a while.

This revision is now accepted and ready to land.Jan 5 2018, 11:16 AM
This revision was automatically updated to reflect the committed changes.