[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:53
+template <>
+unsigned llvm::PotentialConstantIntValuesState::MaxPotentialValues = 0;
+
----------------
okura wrote:
> okura wrote:
> > fhahn wrote:
> > > okura wrote:
> > > > fhahn wrote:
> > > > > Why not just use `MaxPotentialValues` directly and keep it as a global? There might be a reason for doing so that I am missing, but it seems like similar options (e.g. `MaxHeapToStackSize`) are also just using the global directly.
> > > > AAPotentialValues support only integer values now, and similar AA for pointer values or floating values might be introduced. To implement them, we can use the template class PotentialValuesState. I think it is better to be able to set a different threshold for each of them. That is why I keep it as a member variable of PotentialValuesState.
> > > But if you want to set them separately, you'd need separate options?
> > Yes, but if we give a different name for each threshold (e.g. MaxPotentialConstantIntValues, MaxPotentialFloatingValues, etc.), I think it is difficult to commonize implementations into `PotentialValuesState`. I tried to make `PotentialValuesState` take threshold as a template parameter, but I failed to do that because `MaxPotentialValue` cannot be a const variable.
> > FWIW, this option is used in Attributor.h unlike the other options declared in here and we need external storage.  Due to this, `MaxPotentialValue` cannot be const.
> > 
> > If you have an idea, could you tell me that?
> > 
> > OTOH, I confirmed that I could successfully build the current patch with both clang and g++. If we don't necessarily have to change implementation, I can land this anytime.
> @fhahn Could you give me your opinion?
> 
> If there is no response for a while, I will land this.
Oh right, now I see what you mean I think. I still think it would be better to only add the extra complexity once it is needed, but if it builds now I guess it's fine.


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

https://reviews.llvm.org/D83283