This makes it more clear that it's only expected for vector inputs, as opposed to using a hardcoded "APInt(1, 1)".
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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 BitfieldThough 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.
| llvm/include/llvm/Analysis/ValueTracking.h | ||
|---|---|---|
| 65–69 | 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. | |
Comment Actions
The Query is supposed to be constant across the recursion.
I'd like to see something that more closely approximates a proper sum type (if you'll forgive me for posting some haskell here):
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.
| llvm/include/llvm/Analysis/ValueTracking.h | ||
|---|---|---|
| 65–69 | I meant to write "This overload is defined on fixed vectors of integer or pointer type." Not sure it's useful to expose whatever abstraction computeKnownBits is using internally. | |
Comment Actions
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.
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.