Page MenuHomePhabricator

[SVE]Clarify TypeSize comparisons in llvm/lib/Transforms
ClosedPublic

Authored by CarolineConcatto on Oct 19 2020, 8:12 AM.

Details

Summary

Use isKnownXY comparators when one of the operands can be with
scalable vectors or getFixedSize() for all the other cases.

This patch also does bug fixes for getPrimitiveSizeInBits by using
getFixedSize() near the places with the TypeSize comparison.

Diff Detail

Event Timeline

CarolineConcatto requested review of this revision.Oct 19 2020, 8:12 AM
david-arm added inline comments.Oct 19 2020, 8:37 AM
llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
2023–2024

I think it's safe to use getFixedSize() here instead of TypeSize::isKnownLT because we know from the checks just above that these types must be integer types.

sdesmalen added inline comments.Oct 19 2020, 9:10 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
1883–1884

Globals are not allowed to be scalable, so this shouldn't use isKnownLE.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
899–900

Does this need to work for scalable types?

llvm/lib/Transforms/Scalar/NaryReassociate.cpp
378–379

This is a functional change that now allows mixing scalable and fixed-width types. Either that should not happen (and thus it shouldn't use isKnownLT) or it should have a test that tests the new behaviour.

-address review's comments

Thank you Sander and David.
I've changed the points you've raised.
I had to run some test to make sure no scalable vectors would reach the places you've raised.
We can relax, atm no scalable vectors can reach the paths where we are using getFixedSize().

llvm/lib/Transforms/IPO/GlobalOpt.cpp
1883–1884

Thank you Sander.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
899–900

After running some tests I can confirm this path will never be accessible by a scalable vector.
The test inside Transforms/LoopIdiom/memcpy-vectors.ll confirms that this patch will never be reached.

llvm/lib/Transforms/Scalar/NaryReassociate.cpp
378–379

No vector(fix-width or scalable) will go down this path
The test:
if (SE->isSCEVable(I->getType()) && isPotentiallyNaryReassociable(&*I))
inside the method:
bool NaryReassociatePass::doOneIteration(Function &F)
prevents these types to reach this path.

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
2023–2024

Good catch @david-arm.
Thank you for spending the time on it.

CarolineConcatto retitled this revision from [SVE][ValueTracking] Clarify TypeSize comparisons in llvm/lib/Transforms to [SVE]Clarify TypeSize comparisons in llvm/lib/Transforms.Oct 21 2020, 8:11 AM
sdesmalen accepted this revision.Oct 21 2020, 8:25 AM

LGTM!

llvm/lib/Transforms/Scalar/NaryReassociate.cpp
378–379

Okay that makes sense, isSCEVable returns false on vector types. Thanks for confirming!

This revision is now accepted and ready to land.Oct 21 2020, 8:25 AM
This revision was landed with ongoing or failed builds.Oct 23 2020, 1:15 AM
This revision was automatically updated to reflect the committed changes.