[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3184
+  /// See AbstractState::isAtFixpoint()
+  bool isAtFixpoint() const override { return Assumed.isAtWorstState; }
+
----------------
okura wrote:
> 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?
If there is no reason (right now) to keep them separate, merge them. Keep it a template though, I think.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7250
+  }
+};
+
----------------
okura wrote:
> 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` ?
The benefit usually is that `initialize` and/or `updateImpl` are the same or we want to do something special for the call site argument case and fall back to the floating case if it didn't work. So basically, if you can/want to reuse the floating logic somehow, it makes sense to inherit from the floating AA. 

Take `struct AAValueSimplifyCallSiteArgument : AAValueSimplifyFloating` as an example. The "value" is derived the same regardless but the floating algorithm can use the CtxI which is set for the call site argument case.


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

https://reviews.llvm.org/D83283