[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3455
+  static bool classof(const AbstractAttribute *AA) {
+    return (AA->getIdAddr() == &ID);
+  }
----------------
okura wrote:
> fhahn wrote:
> > nit: unnecessary ()
> These brackets are unnecessary indeed, but this function is one of the boilerplate functions of AbstactAttributes and the other AAs have the same implementation. So I think we don't need to remove them.
FWIW, I think this is very uncommon in most parts of the llvm codebase.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83283