[PATCH] D130896: [AA] Tracking per-location ModRef info in FunctionModRefBehavior (NFCI)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D130896#3763481 <https://reviews.llvm.org/D130896#3763481>, @ebrevnov wrote:

> Hi @nikic
>
> Thanks for moving this forward! In general, the direction looks good to me. Before commenting on specific things I would like to discuss one high level thing. I found new FMRB interface a bit overloaded. It's was hard to quickly understand which API  creates new FMRB instance and which makes in place update. My proposal would be:
>
> 1. Make FMRB non-mutable (that would also allow to use static instances for common cases like none, unknown, etc...)

I think this makes sense, and would mostly be a matter of making setModRef private. withModRef (which returns a new instance) is intended as the public API.

> 2. Move location specific API from FMRB to Location. That would separate things out and allow to reduce total number of public API. For example, argMemOnly(ModRefInfo MR) would be not needed as one can simply instantiate FMRB through public constructor.

I'm not sure I understand the suggested API here. Do you mean that instead of `FunctionModRefBehavior::argMemOnly(MR)` we should write `FunctionModRefBehavior(FunctionModRefBehavior::ArgMem, MR)`, or something else?

> 3. Don't introduce withModRef & withoutLoc. Instead model operations through 'operator &' and 'operator |'

I think this would be a bit inconvenient in that manipulating FMRB usually involves first unsetting the old MR for the location and then setting the new one. So `FMRB.withModRef(Loc, NewMR)` would have to become something like `FMRB & ~FunctionModRefInfo(Loc, ModRefInfo::ModRef) | FunctionModRefInfo(Loc, NewMR)`, which is a bit of a mouthful.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130896/new/

https://reviews.llvm.org/D130896