This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Fix invalid usages of getNumElements() in ValueTracking
AbandonedPublic

Authored by ctetreau on Apr 30 2020, 1:34 PM.

Details

Summary

Fix invalid usage of getNumElements in ComputeNumSignBits.
getNumElements was being used to compute the DemandedElts. Since we
can't know the length of a scalable vector, we will instead ignore the
value of DemandedElts, and bail out in any codepath that needs it.

Fix invalid usages of getNumElements in
computeNumSignBitsVectorConstant. Bail out early if the vector is
scalable.

Identified by test case LLVM.Transforms/InstCombine::nsw.ll

Diff Detail

Event Timeline

ctetreau created this revision.Apr 30 2020, 1:34 PM
Herald added a reviewer: efriedma. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
ctetreau updated this revision to Diff 261341.Apr 30 2020, 1:40 PM

minor stylistic changes

Is there a way we can test/guard this?

llvm/lib/Analysis/ValueTracking.cpp
378

By checking the result of dyn_cast<FixedVectorType>, this switch will select APInt(1, 1) when Ty is a ScalableVectorType.
I guess this should just bail out when the type is a scalable vector.

2698

This should be !isa<VectorType>, because APInt(1, 1) doesn't work for scalable vectors.

Should this function also bail out early when the type is a scalablevector?

ctetreau marked an inline comment as done.May 1 2020, 11:14 AM
ctetreau added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
378

I did this on purpose for the same reasoning as in D79053.

Though I'm no longer convinced that it will work for that commit either. I'm thinking these might need a bigger rework that I was hoping to do today. Some sort of scalable APInt? Or maybe we just need a polynomial type?

ctetreau abandoned this revision.May 1 2020, 1:04 PM

I'm going to abandon this and roll the changes into D79053.

For now, I'm just going to bail for all things that take a DemandedElts.