Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[IRSim][IROutliner] Adding consistent function attribute merging

Authored by AndrewLitteken on Sep 8 2020, 10:26 AM.



When combining extracted functions, they may have different function attributes. We want to make sure that we do not make any assumptions, or lose any information. This attempts to make sure that we consolidate function attributes to their most general case.


  • llvm/test/Transforms/IROutliner/outlining-compatible-and-attribute-transfer.ll
  • llvm/test/Transforms/IROutliner/outlining-compatible-or-attribute-transfer.ll

Diff Detail

Event Timeline

AndrewLitteken created this revision.Sep 8 2020, 10:26 AM
Herald added a project: Restricted Project. · View Herald Transcript
AndrewLitteken requested review of this revision.Sep 8 2020, 10:26 AM
jdoerfert added inline comments.Sep 8 2020, 11:13 AM

Are we sure we want/need to duplicate this here in addition to the merge rules at the end of llvm/include/llvm/IR/ ?

AndrewLitteken added inline comments.Sep 8 2020, 12:19 PM

We can probably just call mergeFnAttrs from this function and merge the attributes from SanitizeAddress separately.

jdoerfert added inline comments.Sep 8 2020, 12:26 PM

Hm, why are the "SanitizeAddress" address ones not handled in mergeFnAttrs as well? Doesn't the inliner need to know about them too? I would prefer any way we avoid a second list, that is doomed to cause problems.

AndrewLitteken added inline comments.Sep 8 2020, 12:34 PM

I think because it uses the areInlineCompatible, like above, where I'm merging them for combining two outlined functions functions together and make sure that the information isn't lost.

That said, I'm not super familiar with these attributes, so this may not be considered safe.

Updating function attribute handling to use existing functions.

jdoerfert added inline comments.Sep 15 2020, 10:02 PM

I don't get it. Why can't we call mergeFnAttrs(Caller, Callee); here?


It should be, I think I uploaded the wrong diff. Sorry about that.

This revision is now accepted and ready to land.Sep 15 2020, 10:22 PM