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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added a comment.

In D130896#3763527 <https://reviews.llvm.org/D130896#3763527>, @nikic wrote:

> 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.

Cool.

>> 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?

Yep, something like that, except, I was thinking about keeping FunctionModRefLocation top level and calling its member FunctionModRefLocation::argMemOnly() to get location.

>> 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.

Agree. Maybe we can find a bit better name(s). In particular 'withoutLoc' looks confusing.


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

https://reviews.llvm.org/D130896