This is an archive of the discontinued LLVM Phabricator instance.

[AA] Rename FunctionModRefBehavior (NFC)
ClosedPublic

Authored by nikic on Oct 14 2022, 8:10 AM.

Details

Summary

As part of https://discourse.llvm.org/t/rfc-unify-memory-effect-attributes/65579, the FunctionModRefBehavior class sees a good bit of additional use, and I've found the name to be something of a mouthful. I'd like to rename it to something more concise. I have two suggestions:

  • FunctionModRefBehavior -> ModRefBehavior. I don't think the Function prefix is adding anything here, and I think we will actually want to use this class to model memory effects of non-call instructions as well. While most non-call instructions are argmemonly ("arg" in the sense of "instruction operand" here), this is not true for volatile operations and other special instructions like fences, and having central modelling for this would prevent mistakes like D135863.
  • FunctionModRefBehavior -> MemoryEffects (what this patch currently does). This would be in line with the terminology of the aforementioned RFC. I think this term is more obvious to people who are not familiar with our AA lingo.

Happy to bikeshed other variants though :)

This patch currently just updates the class definition. If we do the rename, I would of course also update other references. For the second option, I'd also rename getModRefBehavior() methods to getMemoryEffects().

Diff Detail

Event Timeline

nikic created this revision.Oct 14 2022, 8:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2022, 8:10 AM
nikic requested review of this revision.Oct 14 2022, 8:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2022, 8:10 AM

I am in favor of this version. I was already not happy with the new uses of the long "FunctionModRefBehavior" for the memory attribute, this makes it shorter and less confusing (IMHO).

fhahn added a comment.Oct 14 2022, 2:07 PM

MemoryEffects sounds good to me.

nikic added a comment.Oct 17 2022, 2:11 AM

As there seems to be support for the MemoryEffects name, I plan to land this tomorrow, if there are no further objections.

jdoerfert accepted this revision.Oct 17 2022, 9:00 AM
This revision is now accepted and ready to land.Oct 17 2022, 9:00 AM
fhahn accepted this revision.Oct 18 2022, 2:49 AM

LGTM, thanks!

This revision was landed with ongoing or failed builds.Oct 19 2022, 1:38 AM
This revision was automatically updated to reflect the committed changes.