This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Add new isKnownXX comparison functions to TypeSize
ClosedPublic

Authored by david-arm on Sep 22 2020, 7:17 AM.

Details

Summary

This patch introduces four new comparison functions:

isKnownLT, isKnownLE, isKnownGT, isKnownGE

that return true if we know at compile time that a particular
condition is met, i.e. that one size is definitely greater than
another. The existing operators <,>,<=,>= remain in the code for
now, but over time we would like to remove them and change the
code to use the isKnownXY routines instead. These functions do
not assert like the existing operators because the caller is
expected to properly deal with cases where we return false by
analysing the scalable properties. I've made more of an effort
to deal with cases where there are mixed comparisons, i.e. between
fixed width and scalable types.

I've also added some knownBitsXY routines to the EVT and MVT
classes that call the equivalent TypeSize::isKnownXY routines.
I've changed the existing bitsXY functions to call their knownBitsXY
equivalents and added asserts that the scalable properties match.
Again, over time we expect to migrate callers to use knownBitsXY
and make the code more aware of the scalable nature of the sizes.

Diff Detail

Event Timeline

david-arm created this revision.Sep 22 2020, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2020, 7:17 AM
david-arm requested review of this revision.Sep 22 2020, 7:17 AM

I think this is fine, but I went cross-eyed verifying the conditionals. Somebody else should double-check that the conditionals are fine.

paulwalker-arm accepted this revision.Sep 23 2020, 4:06 AM

My preference would be to follow the clang-format advice for TypeSize.h.

Is there value is adding isKnownEQ just in case somebody wants to use function pointers?

llvm/include/llvm/Support/TypeSize.h
190–195

I'm happy with this but wondered if the following might reduce the complexity.

return isKnownLT(LHS, RHS) || isKnownEQ(LHS, RHS);
This revision is now accepted and ready to land.Sep 23 2020, 4:06 AM
paulwalker-arm added inline comments.Sep 23 2020, 9:04 AM
llvm/include/llvm/Support/TypeSize.h
190–195

As discussed, I can see that my suggestion actually covers less cases, so is not the way to go.

This revision was landed with ongoing or failed builds.Sep 24 2020, 2:23 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen added inline comments.Sep 24 2020, 9:57 AM
llvm/include/llvm/Support/TypeSize.h
176

Sorry to spot this only after you've landed the patch, but was there an explicit reason for making these functions static as opposed to allowing the shorter X.isKnownLT(Y) (that wouldn't need the TypeSize:: or ElementCount:: prefix)?

david-arm added inline comments.Sep 24 2020, 11:58 PM
llvm/include/llvm/Support/TypeSize.h
176

So it just seemed like a much more natural replacement for the existing operators, i.e. friend functions with a LHS and RHS:

friend bool operator<(const TypeSize &LHS, const TypeSize &RHS)

Also, I can see value in introducing the operators this way as in future once we have a templated PolySize we might want these operators to be more generic, such as:

template <typename X, typename Y>
bool SomeNameSpace::isKnownGT(const PolySize<X> LHS, const PolySize<Y> RHS);

or even have the LHS not be a PolySize at all (i.e. LHS is a simple integer):

template <typename X, typename Y>
bool SomeNameSpace::isKnownGT(X LHS, const PolySize<Y> RHS) {

PolySize<X> LHS2 = PolySize::getFixed(LHS);
return isKnownGT(LHS2, RHS);

}

and so on.