Page MenuHomePhabricator

[VPlan] Add initial VPlan verification.
ClosedPublic

Authored by fhahn on Oct 7 2021, 4:28 AM.

Details

Summary

This patch adds a function to verify general properties of VPlans. The
first check makes sure that all phi-like recipes are at the beginning of
a block, with no other recipes in between.

Note that this currently may not hold for VPBlendRecipes at the moment,
as other recipes may be inserted before the VPBlendRecipe during mask
creation.

Note that this patch depends on D111300 and D111301, which fix code that
breaks the checked invariant.

Diff Detail

Event Timeline

fhahn created this revision.Oct 7 2021, 4:28 AM
fhahn requested review of this revision.Oct 7 2021, 4:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2021, 4:28 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added a comment.Oct 25 2021, 2:26 PM

Looks good to me, thanks!
Perhaps also worth checking that all header-phi recipes reside in the header?

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
148

worth spelling out the offenders?

fhahn updated this revision to Diff 382601.Oct 27 2021, 4:17 AM

Looks good to me, thanks!
Perhaps also worth checking that all header-phi recipes reside in the header?

That sounds like a good idea. I'll put up a follow-up patch, to keep the patches smaller in case they highlight any issues.

Ayal accepted this revision.Tue, Nov 9, 1:07 AM

Thanks for pushing this forward!

Wondering about:

/// Note that currently there is still an exception for VPBlendRecipes.

// FIXME: At the moment, creating a mask might insert non-phi recipes before VPBlendRecipes.

VPBlendRecipes are indeed exceptional (not only currently?), as they represent a 'select' rather than a 'phi'. Is the FIXME needed?

This revision is now accepted and ready to land.Tue, Nov 9, 1:07 AM
This revision was landed with ongoing or failed builds.Tue, Nov 9, 2:18 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.
akuegel added a subscriber: akuegel.Tue, Nov 9, 2:34 AM
akuegel added inline comments.
llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
147

The dump() method is not necessarily available, it is guarded by:
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)

You should guard the calls to it in the same way.

fhahn added a comment.Tue, Nov 9, 2:36 AM

Thanks for pushing this forward!

Wondering about:

/// Note that currently there is still an exception for VPBlendRecipes.

// FIXME: At the moment, creating a mask might insert non-phi recipes before VPBlendRecipes.

VPBlendRecipes are indeed exceptional (not only currently?), as they represent a 'select' rather than a 'phi'. Is the FIXME needed?

Good point, I removed the fixme in the committed version.

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
147

Thanks, this should already be fixed by acbefbf19f6c

148

Thanks, I updated the code to print the phi-like recipe and the previous (non-phi like)

akuegel added inline comments.Tue, Nov 9, 2:36 AM
llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
147

Ok, I see your commit now :)
Thanks for the fix.