Page MenuHomePhabricator

[VPlan] Use VPDominatorTree in VPlanVerifier .
ClosedPublic

Authored by fhahn on Dec 21 2022, 3:28 PM.

Details

Summary

Use VPDominatorTree to generalize def-use verification.

Depends on D140513.

Diff Detail

Event Timeline

fhahn created this revision.Dec 21 2022, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2022, 3:28 PM
fhahn requested review of this revision.Dec 21 2022, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2022, 3:28 PM
Ayal added inline comments.Thu, Jan 19, 1:59 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9164 ↗(On Diff #484706)

nit: can alternatively have the verifier construct a dominator tree? Once one is used for non-verification purposes, it could be passed in, to save a recalculation.

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
209–210

Blend recipe should adhere to def-before-use?
PredInstPHI should be considered a header phi?
Worth having a test to check the verifier?

216

nit: can early-continue (independent of this patch).

237–238

nit: can use utils to iterate over basic blocks only, now that regions needs not be assigned numbers.

fhahn updated this revision to Diff 491770.Tue, Jan 24, 7:06 AM

Rebased on top of new unit tests (dc8e2ea92953a6) and addressed comments, thanks!

fhahn marked 4 inline comments as done.Tue, Jan 24, 7:10 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9164 ↗(On Diff #484706)

Done, thanks!

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
209–210

Blend recipe should adhere to def-before-use?

Yep, updated the condition to not skip blend recipes

PredInstPHI should be considered a header phi?

Not sure if it makes sense to classify PredInstPHI as header-phi, but the verifier needs extending so to check the incoming values dominate the end of the incoming block. Will put up a patch for that separately.

Worth having a test to check the verifier?

Added initial tests in dc8e2ea9 and update the patch to show improvements.

216

I kept the order for now as is, so the check directly relates to the error message.

226

I removed that unrelated change from the diff, will submit a separate patch for that.

237–238

Done, thanks!

Ayal accepted this revision.Tue, Jan 24, 8:31 AM

Ship it!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9165 ↗(On Diff #491770)

nit: can be done independent of this patch.

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

nit: can introduce a method that takes two recipes (along with their numberings) and says if one dominates the other, whether they belong to the same VPBB's or not. Analogous to DominatorTree interfaces.

This revision is now accepted and ready to land.Tue, Jan 24, 8:31 AM
fhahn marked 6 inline comments as done.Wed, Jan 25, 4:17 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9165 ↗(On Diff #491770)

It's not needed any longer now that the VPDominatorTree is computed inside the call. I'll remove it.

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

That would indeed be a useful utility. I'll prepare that separately.

fhahn updated this revision to Diff 492064.Wed, Jan 25, 4:28 AM
fhahn marked 2 inline comments as done.

rebase before landing.

fhahn updated this revision to Diff 492065.Wed, Jan 25, 4:29 AM

Remove unnecessary ifdef change.

This revision was automatically updated to reflect the committed changes.