This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Bail from VectorUtils heuristics for scalable vectors
ClosedPublic

Authored by ctetreau on Sep 9 2020, 4:29 PM.

Details

Summary

Bail from maskIsAllZeroOrUndef and maskIsAllOneOrUndef prior to iterating over the number of
elements for scalable vectors.

Assert that the mask type is not scalable in possiblyDemandedEltsInMask .

Assert that the types are correct in all three functions.

Diff Detail

Event Timeline

ctetreau created this revision.Sep 9 2020, 4:29 PM
ctetreau requested review of this revision.Sep 9 2020, 4:29 PM
efriedma added inline comments.Sep 9 2020, 5:46 PM
llvm/lib/Analysis/VectorUtils.cpp
920–925

I think I'd prefer to handle this in the callers: if the vector is scalable, just don't call it. Returning an Optional<> is more confusing, I think, since it can't fail for fixed vectors.

efriedma added inline comments.Sep 9 2020, 5:47 PM
llvm/include/llvm/Analysis/VectorUtils.h
551–552

If the input isn't i1, it isn't obvious what "true"/"false" means here.

ctetreau added inline comments.Sep 10 2020, 9:24 AM
llvm/include/llvm/Analysis/VectorUtils.h
551–552

I guess these functions make and don't check lots of assumptions. (element type is bool, type is not just int, etc...) I'm just trying to follow the boy scout code here and leave the code cleaner than I found it. It would be better if I just asserted that the mask type is a vector and the element type is a bool, then the function would actually do what it says on the tin.

llvm/lib/Analysis/VectorUtils.cpp
920–925

I can change it to assert for scalable vectors. If we ever add a notion of a scalable demanded elements (we do this thing with bitfields in lots of places), then it's possible that this function could return a value for scalable vectors. Though in that event the return type of this function would change anyways, so we might as well wait.

ctetreau updated this revision to Diff 291002.Sep 10 2020, 9:41 AM

address code review issues

ctetreau updated this revision to Diff 291006.Sep 10 2020, 9:47 AM

fix now unnecessary assignment to optional<APInt>

ctetreau marked 2 inline comments as done.Sep 10 2020, 9:50 AM
ctetreau edited the summary of this revision. (Show Details)
ctetreau edited the summary of this revision. (Show Details)
ctetreau updated this revision to Diff 291008.Sep 10 2020, 9:54 AM

remove unneeded include

Harbormaster completed remote builds in B71250: Diff 291006.
This revision is now accepted and ready to land.Sep 10 2020, 11:03 AM