[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Shinji Okumura via Phabricator via llvm-commits]

okura added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:53
+template <>
+unsigned llvm::PotentialConstantIntValuesState::MaxPotentialValues = 0;
+
----------------
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.


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

https://reviews.llvm.org/D83283