[PATCH] D79264: [ValueTracking] Use "Optional" for DemandedElts argument to ComputeKnownBits

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

> The Query is supposed to be constant across the recursion.

Currently it works that way. And if there's a strong desire to keep it that way, then I think the APInt replacement can be a separate object. But I still think just asking the user to construct a query is better than having all these overloads. And For the APInt replacement, I'd like to be able to construct a thing and call a function on in instead of having to study a function comment, then hope I constructed the correct APInt value. It's not *that hard* but it's just another thing to mess up.

> I guess you want to replace Optional<APInt> with something like std::variant<SomethingForScalar, SomethingForScalable, APInt>? That's structurally identical, given there isn't anything to store for scalars, and I'm not actually implementing anything for scalable types here. But I guess the names are different? I can add typedef Optional<APInt> DemandedLaneTy; const APInt& FixedVecLanes(const DemandedLaneTy &DemandedElts) { return *DemandedElts; } if you think that helps.

I see what you're saying, and I personally think the ergonomics of std::variant are extremely poor. But if you're going to make the argument that the variant version is equivalent to the optional version, then I would make the argument that the optional version is equivalent to what we have, and unlike the variant version, it's not clear from a glance what None means.

Really, the idiomatic C++ thing to do would be to have an object. Something like:

  class DemandedElts {
  // don't care what the representation is
  // constructors private
  public:
  enum class DemandType { Scalar, ScalableVector, FixedVector };
  
  static DemandedElts Scalar();
  static DemandedElts ScalableVector();
  static DemandedElts FixedVector(APInt);
  
  DemandType GetDemandType() const;
  APInt GetFixedVectorIndices() const;
  }

This way it can be extended without creating some nested template abomination.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79264