Page MenuHomePhabricator

[SVE] Remove calls to VectorType::getNumElements from Analysis
ClosedPublic

Authored by ctetreau on Jun 9 2020, 2:19 PM.

Diff Detail

Event Timeline

ctetreau created this revision.Jun 9 2020, 2:19 PM
Herald added a project: Restricted Project. · View Herald Transcript
RKSimon added inline comments.Jun 17 2020, 10:17 AM
llvm/lib/Analysis/ConstantFolding.cpp
111

Just use FixedVectorType in the dyn_cast here?

141

dyn_cast<FixedVectorType> ?

llvm/lib/Analysis/InstructionSimplify.cpp
950

VTy is already FixedVectorType?

llvm/lib/Analysis/ValueTracking.cpp
3486–3487

use a dyn_cast<> so we don't have a isa<> and a cast<>

3591–3592

use a dyn_cast<> so we don't have a isa<> and a cast<>

llvm/lib/Analysis/VectorUtils.cpp
815

dyn_cast<FixedVectorType>

ctetreau marked 4 inline comments as done.Jun 17 2020, 10:55 AM
ctetreau added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
111

That would be a behavior change. The idea for this patch is to remove the invalid calls to VectorType::getNumElements with no other behavior changes. The original code enters this branch for scalable vectors, then possibly miscompiles or crashes further down the line. Now it enters this branch for scalable vectors and crashes immediately. As follow up work, cases like this should get looked at.

We're trying to avoid behavior changes of the form "this branch used to be taken for scalable vectors, now it isn't" because 1) There are a ton of such cases, and if I had to write tests for all of them right now this work would never get done and 2) A lot of these cases are actually impossible to trigger for scalable vectors (as of today), so we can't actually write tests for them. I haven't analyzed this particular one, but in general anything that has to do with constants is hard to test with scalable vectors because there are only a few scalable vectors that it's actually possible to construct constants for today.

141

this would be a behavior change

llvm/lib/Analysis/InstructionSimplify.cpp
950

I believe that this is a rebase artifact. I'll fix it.

llvm/lib/Analysis/VectorUtils.cpp
815

this would be a behavior change

ctetreau updated this revision to Diff 271488.Jun 17 2020, 2:28 PM

address code review issues

RKSimon accepted this revision.Jul 21 2020, 3:26 AM

LGTM

This revision is now accepted and ready to land.Jul 21 2020, 3:26 AM
This revision was automatically updated to reflect the committed changes.