[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Shinji Okumura via Phabricator via llvm-commits]

okura added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3455
+  static bool classof(const AbstractAttribute *AA) {
+    return (AA->getIdAddr() == &ID);
+  }
----------------
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.


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


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

https://reviews.llvm.org/D83283