Page MenuHomePhabricator

[LV] Move LoopVersioning creation to LVP::execute.
ClosedPublic

Authored by fhahn on Jun 16 2022, 7:39 AM.

Details

Summary

At the moment LoopVersioning is only created for inner-loop
vectorization. This patch moves it to LVP::execute, which means it will
also be added for epilogue vectorization. As a consequence, the proper
noalias metadata is now also added to epilogue vector loops.

LVer will be moved to VPTransformState as follow-up.

Diff Detail

Event Timeline

fhahn created this revision.Jun 16 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 7:39 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn requested review of this revision.Jun 16 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 7:39 AM
Ayal added a comment.Jun 26 2022, 2:09 PM

Curious if current lack of proper noalias metadata of an epilogue vector loop may result in missed optimization or potentially wrong code?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7616

Above comment still holds, given that here it's not past an if (!MemCheckBlock) early-exit?

C/Should it move to appear before BestVPlan.prepareToExecute(), but presumably must appear after ILV.createVectorizedLoopSkeleton()?

fhahn updated this revision to Diff 440147.Jun 27 2022, 3:15 AM

Add extra check if there are any runtime checks before creating LoopVersioning, move code up.

Curious if current lack of proper noalias metadata of an epilogue vector loop may result in missed optimization or potentially wrong code?

The lack of noalias metadata currently leads to potentially missing optimizations later on. The noalias properties must hold for the epilogue loop, otherwise the vectorization decisions may be incorrect. Missing metadata should never impact correctness, as LLVM optimizations are free to drop it.

fhahn marked an inline comment as done.Jun 27 2022, 3:16 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7616

Above comment still holds, given that here it's not past an if (!MemCheckBlock) early-exit?

The earlier version unnecessarily created LoopVersioning, I updated the code to only create it if there are *any* checks. This should be similar to the earlier exit in the original code and avoid unnecessary work.

C/Should it move to appear before BestVPlan.prepareToExecute(), but presumably must appear after ILV.createVectorizedLoopSkeleton()?

I moved the code up.

Ayal added a comment.Jun 27 2022, 9:48 AM

Add extra check if there are any runtime checks before creating LoopVersioning, move code up.

Great, thanks!
Currently noalias is introduced by emitMemRuntimeChecks() when called from ILV::createVectorizedLoopSkeleton() and from its overriding EpilogueVectorizerMainLoop::createEpilogueVectorizedLoopSkeleton(), but is missed by its overriding EpilogueVectorizerEpilogueLoop::createEpilogueVectorizedLoopSkeleton(). How about placing it in completeLoopSkeleton(), which all three skeleton creators call? It does, after all, seem to belong to ILV* than to LVP?

Curious if current lack of proper noalias metadata of an epilogue vector loop may result in missed optimization or potentially wrong code?

The lack of noalias metadata currently leads to potentially missing optimizations later on. The noalias properties must hold for the epilogue loop, otherwise the vectorization decisions may be incorrect. Missing metadata should never impact correctness, as LLVM optimizations are free to drop it.

OK, good, sure; just making sure "lack of proper" does not mean "existence of improper".

fhahn marked an inline comment as done.Jun 27 2022, 10:09 AM

Add extra check if there are any runtime checks before creating LoopVersioning, move code up.

Great, thanks!
Currently noalias is introduced by emitMemRuntimeChecks() when called from ILV::createVectorizedLoopSkeleton() and from its overriding EpilogueVectorizerMainLoop::createEpilogueVectorizedLoopSkeleton(), but is missed by its overriding EpilogueVectorizerEpilogueLoop::createEpilogueVectorizedLoopSkeleton(). How about placing it in completeLoopSkeleton(), which all three skeleton creators call? It does, after all, seem to belong to ILV* than to LVP?

The follow-up patches move addMetadata to VPTransformState, which requires VPTransformState to have access to LoopVersioning. If we move the code to completeLoopSkeleton, it would have to be updated to return the LoopVersioning. Please let me know if you prefer it that way and I'll updated the patch :)

Ayal accepted this revision.Jun 28 2022, 2:19 AM

Add extra check if there are any runtime checks before creating LoopVersioning, move code up.

Great, thanks!
Currently noalias is introduced by emitMemRuntimeChecks() when called from ILV::createVectorizedLoopSkeleton() and from its overriding EpilogueVectorizerMainLoop::createEpilogueVectorizedLoopSkeleton(), but is missed by its overriding EpilogueVectorizerEpilogueLoop::createEpilogueVectorizedLoopSkeleton(). How about placing it in completeLoopSkeleton(), which all three skeleton creators call? It does, after all, seem to belong to ILV* than to LVP?

The follow-up patches move addMetadata to VPTransformState, which requires VPTransformState to have access to LoopVersioning. If we move the code to completeLoopSkeleton, it would have to be updated to return the LoopVersioning. Please let me know if you prefer it that way and I'll updated the patch :)

OK, and having LVP take care of creating LVer (after it creates State and calls ILV* to create the skeleton) and passing it into State is one way of achieving this.
We should find a better way for State to take care of all metadata, and if/how LoopVersioning is reused for this; would be good to leave behind a TODO.

This revision is now accepted and ready to land.Jun 28 2022, 2:19 AM
This revision was landed with ongoing or failed builds.Jun 30 2022, 4:14 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jun 30 2022, 4:14 AM

Add extra check if there are any runtime checks before creating LoopVersioning, move code up.

Great, thanks!
Currently noalias is introduced by emitMemRuntimeChecks() when called from ILV::createVectorizedLoopSkeleton() and from its overriding EpilogueVectorizerMainLoop::createEpilogueVectorizedLoopSkeleton(), but is missed by its overriding EpilogueVectorizerEpilogueLoop::createEpilogueVectorizedLoopSkeleton(). How about placing it in completeLoopSkeleton(), which all three skeleton creators call? It does, after all, seem to belong to ILV* than to LVP?

The follow-up patches move addMetadata to VPTransformState, which requires VPTransformState to have access to LoopVersioning. If we move the code to completeLoopSkeleton, it would have to be updated to return the LoopVersioning. Please let me know if you prefer it that way and I'll updated the patch :)

OK, and having LVP take care of creating LVer (after it creates State and calls ILV* to create the skeleton) and passing it into State is one way of achieving this.
We should find a better way for State to take care of all metadata, and if/how LoopVersioning is reused for this; would be good to leave behind a TODO.

Thanks, I added a TODO in the committed version!