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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

Honestly, I don't think this is an improvement. Before, DemandedElts was a bitfield, and for scalars the bitfield had a width of 1. While this doesn't account for scalable vectors, it makes sense from the perspective of "a vector with length of 1 is a scalar." Now we have the ability to pass None here. What does None even mean?

I'd like to see something that more closely approximates a proper sum type (if you'll forgive me for posting some haskell here):

  data DemandedElts = Scalar
                    | ScalableVector {- stuff to customize how it's interpreted -}
                    | FixedVector Bitfield

Though really, maybe it makes sense to just expose Query, and get rid of all of these overloads. DemandedElts can move into Query, and it could wrap the bitfield and add some niceties:

  Query Q(DL);
  Q.SetDemandedEltsScalable();
  KnownBits KB = computeKnownBits(V, Q);

Query could have some internal representation that callers of computeKnownBits don't have to care about. All of those overloads can go away, and so can their hundreds of default parameters.



================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:65-69
+  /// This overload is defined on vectors of integer or pointer type.
+  /// DemandedElts has the width of the vector; each bit indicates whether that
+  /// element of the vector is demanded. The known zero and known one values
+  /// are the same width as the vector element, and the bit is set only if it
+  /// is true for all of the demanded elements in the vector.
----------------
This doesn't actually mention scalable vectors, and to me it reads like the caller is supposed to pass a value for them (not None).

Also, this actually is defined for scalars; it just assumes you've precomputed the DemandedElts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79264