[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Shinji Okumura via Phabricator via llvm-commits]

okura marked 2 inline comments as done.
okura added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3184
+  /// See AbstractState::isAtFixpoint()
+  bool isAtFixpoint() const override { return Assumed.isAtWorstState; }
+
----------------
jdoerfert wrote:
> This "tracks" only the worst state fixpoint but not any other one, e.g., if it is a known constant. Are you sure we don't need a separate boolean to track this. I would have assumes so to avoid calling `update` on the AA after we used `indicateOptimisticFixpoint`.
> 
> One way to handle this is to replace `isAtWorstState` with a `BooleanState` which handles the fixpoint and the valid bit. You can probably even derive from that class to minimize the code you need to type, e.g., a lot of functions provided by `BooleanState` might then just be good enough. Only once you need to inspect or touch the Set you have to provide functions here.
I do not notice that it became unable to track fixpoint in the optimistic case by eliminating Known state. I changed this to derive from `BooleanState` as you suggest.
By this, I feel that `PotentialValuesState` becomes a simple (unnecessary) wrapper for `PotentialValueSet`. Do you think I have to merge two structs together?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7250
+  }
+};
+
----------------
jdoerfert wrote:
> Would it make sense to let `AAPotentialValuesCallSiteArgument` derive from `AAPotentialValuesFloating`? We often do that.
I will probably override both `initialize` and `updateImpl` in `AAPotentialValuesCallSiteArgument`. Even so, is deriving from `AAPotentialValuesFloating`  better than from `AAPotentialValuesImpl` ?


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

https://reviews.llvm.org/D83283