This is an archive of the discontinued LLVM Phabricator instance.

[LV] Do not LoopSimplify/LCSSA after generating main vector loop.
ClosedPublic

Authored by fhahn on May 17 2022, 9:59 AM.

Details

Summary

At the moment LV runs LoopSimplify and reconstructs LCSSA form after
generating the main vector loop and before generating the epilogue
vector loop.

In practice, this adds a new exit block for the scalar loop because the
middle block now also branches to the original exit block of the scalar
loop. It also requires adding a new LCSSA phi in the newly created exit
block.

This complicates things when modeling exit values in VPlan, because we
would need to update the VPlan for the epilogue loop to update the newly
created LCSSA phi node.

But none of that should be necessary, as all analysis requiring
loop-simplify form is already done at this point and LCSSA form of the
original loop is not broken.

Diff Detail

Event Timeline

fhahn created this revision.May 17 2022, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 9:59 AM
fhahn requested review of this revision.May 17 2022, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 9:59 AM

IIRC the reason for calling formLCSSARecursively was to make sure we can handle live outs and reductions properly, which we seem to still be able to do (based on LIT test updates). The call to simplifyLoop was to ensure we don't trip any assumptions about loop simplify form when processing the epilogue loop. It may not be an issue now, but would it be a problem, say if a hypothetical utility function common to both paths wants to assert that the loop is in simplified form? Is keeping simplifyLoop() harmless to VPlan's modeling of exit values?

fhahn added a comment.May 17 2022, 2:26 PM

IIRC the reason for calling formLCSSARecursively was to make sure we can handle live outs and reductions properly, which we seem to still be able to do (based on LIT test updates).

Thank you very much for checking! I *think* that should be save, as any checks should only rely on LCSSA phis in the original scalar loop and that loop remains unchanged. We add incoming values to the LCSSA phis, but that should leave LCSSA form of the original scalar loop intact.

The call to simplifyLoop was to ensure we don't trip any assumptions about loop simplify form when processing the epilogue loop. It may not be an issue now, but would it be a problem, say if a hypothetical utility function common to both paths wants to assert that the loop is in simplified form? Is keeping simplifyLoop() harmless to VPlan's modeling of exit values?

I think it is actually simplifyLoop that's the main issue. It will create a new exit block that only has predecessors in the scalar loop. After code generation for the main vector loop, the original exit block will have predecessor outside the scalar loop (the "middle" block). And for the new exit block we need to fix LCSSA form for this block. Those new LCSSA phis would complicate things in D123537, because the VPlan would still reference the old LCSSA phi nodes, whereas we would need to update the new LCSSA nodes when generating code for the epilogue.

Over the last few years, LV has continually moved towards materializing all information required for code-generation in VPlan and I think introducing a new reliance on loop-simplify form during vector code generation would be problematic in general, as it would introduce a new dependence on the original IR. In terms of current code-generation it should be fine, as we don't actually generate code in the exit block of the scalar loop, we just update the phis.

bmahjour accepted this revision.May 19 2022, 7:02 AM

Ok, LGTM. Thanks!

This revision is now accepted and ready to land.May 19 2022, 7:02 AM
This revision was landed with ongoing or failed builds.May 20 2022, 1:59 AM
This revision was automatically updated to reflect the committed changes.