This is an archive of the discontinued LLVM Phabricator instance.

Force Remove Attribute
ClosedPublic

Authored by kyulee on Aug 8 2020, 12:59 PM.

Details

Summary

-force-attribute adds an attribute to function via command-line.
However, there was no counter-part to remove an attribute.
This patch adds -force-remove-attribute that removes an attribute from function.

Diff Detail

Event Timeline

kyulee created this revision.Aug 8 2020, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2020, 12:59 PM
kyulee requested review of this revision.Aug 8 2020, 12:59 PM
kyulee edited the summary of this revision. (Show Details)
kyulee updated this revision to Diff 284149.Aug 8 2020, 3:28 PM

Fix case style for ParseFunctionAndAttr

Kindly ping.. This patch is convenient to chase down an issue by adding as well as removing an attribute of the function.

Overall this change makes sense. I don't see why we shouldn't have this ability to remove attrs.

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
22–34

Hmm, do we care that we are changing the command line flag? Should -force-attribute imply -force-add-attribute?

31

"from a function"

81

Spell: *takes precedence.

112

Introduce hasForceAttributes as a separate NFC patch.

plotfi added inline comments.Aug 10 2020, 6:51 PM
llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
22–34

@chandlerc Do you think -force-attribute should be preserved for any existing scripts or automation that may assume this?

kyulee updated this revision to Diff 284826.Aug 11 2020, 11:04 AM

Fix typos from the feedback.

kyulee marked 2 inline comments as done.Aug 11 2020, 11:13 AM
kyulee added inline comments.
llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
22–34

@chandlerc @plotfi I could preserve the original name. Then I think we can have these two - force-attribute and remove-attribute.
I don't mind either direction about naming. Let me know.

112

Do you mean like this? Isn't it too simple without additional checks like this?

static bool hasForceAttributes() {
  return !ForceAttributes.empty();
}
plotfi added inline comments.Aug 11 2020, 11:55 AM
llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
112

Yeah, just mention its to make this diff simpler. I'll approve it.

Adding jmolloy, he seems to be the original author.

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
22–34

I think the renaming isn't necessary -- it'll break any downstream users with no benefit. -force-attribute and -force-remove-attribute are a satisfactory pairing.

kyulee updated this revision to Diff 284924.Aug 11 2020, 3:48 PM
  • Preserve the original flag -force-attribute
  • Factor out hasForceAttributes into D85793
kyulee retitled this revision from Force Function Attributes to Force Remove Attribute.Aug 11 2020, 3:50 PM
kyulee edited the summary of this revision. (Show Details)
kyulee marked 3 inline comments as done.Aug 11 2020, 4:25 PM

Please Update Diff.

I think this LGTM. @kyulee are there any other tests that use this flag? Can you add a test that both adds and also removes some different attributes. Also a test that adds and removes he same attribute too would be good.

kyulee updated this revision to Diff 286403.Aug 18 2020, 2:47 PM

Rebase and add more tests.

plotfi accepted this revision.Aug 19 2020, 12:29 PM

Ok, LGTM. I will land for you, but in your name on git. Thanks for this patch @kyulee.

This revision is now accepted and ready to land.Aug 19 2020, 12:29 PM
This revision was automatically updated to reflect the committed changes.