[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;
+
----------------
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.


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

https://reviews.llvm.org/D83283