This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Manage induction value creation using VPValues.
ClosedPublic

Authored by fhahn on Nov 29 2020, 1:46 PM.

Details

Summary

This patch updates the induction value creation to use VPValues of
recipes to map the created values. This should bring is one step closer
to being able to optimize induction recipes directly in VPlan.

Currently widenIntOrFpInduction also generates vector values for a cast
of the induction, if it exists. Make this explicit by adding the cast
instruction to the values defined by the recipe.

Diff Detail

Event Timeline

fhahn created this revision.Nov 29 2020, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2020, 1:46 PM
fhahn requested review of this revision.Nov 29 2020, 1:46 PM
gilr added a comment.Jan 19 2021, 2:39 PM

Should the (final) goal be to move the whole phi-trunc-cast logic from code-gen to vplanning phase?

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

Delete

2159–2160

Isn't extracting CastInst here from ID redundant now, given CastDef which wraps it?

2160

&& "error message"

9008

Can IsTrunc be replaced by dyn_cast<>? (and isa<> in print()?)

9009

Might be good to have named getters for the trunc and the cast defs.

9260
  • Make this an early exit?
  • Worth leaving a TODO for replacing the IF with an assertion once all scalar recipes are VPDef'ed and throwing away that callback.
fhahn updated this revision to Diff 318843.Jan 24 2021, 7:47 AM

Address comments, thanks!

fhahn marked 4 inline comments as done.Jan 24 2021, 7:49 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2160

I removed the assert as it should not be needed & also removed CastInst.

9009

Added helpers and removed IsTrunc field.

9260

The early exit is much better & I also added a TODO. thanks!

fhahn marked 2 inline comments as done.Jan 24 2021, 8:51 AM

Should the (final) goal be to move the whole phi-trunc-cast logic from code-gen to vplanning phase?

Yes I think so, I'm hoping we can simplify some of the logic handling inductions a bit by moving things to the planning phase.

fhahn added inline comments.Jan 25 2021, 7:27 AM
llvm/lib/Transforms/Vectorize/VPlan.cpp
219–220

I had to add this as part of 3201274dea63, because otherwise there would be some failures for VPValue that were inductions. This is fixed with this patch, so this can be removed again. The OrigLoop field can also be removed again.

fhahn added inline comments.Jan 25 2021, 11:49 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9260

I put up a patch to remove the getOrCreateVectorValues call and the whole VPCallback: D95383 :)

fhahn updated this revision to Diff 319870.Jan 28 2021, 7:54 AM

Rebased & ping. @gilr does this look good after the recent changes? :)

gilr accepted this revision.Feb 3 2021, 8:41 AM

LGTM with a nit. Thanks!

llvm/lib/Transforms/Vectorize/VPlan.h
940

Doesn't seem to be used anywhere (The start argument going into the VPUser)

This revision is now accepted and ready to land.Feb 3 2021, 8:41 AM
fhahn updated this revision to Diff 321129.Feb 3 2021, 9:40 AM

rebased & removed unnecessary leftover VPValue *Start;

Thanks!

fhahn updated this revision to Diff 321132.Feb 3 2021, 9:44 AM

Another rebase on top of main

This revision was landed with ongoing or failed builds.Feb 3 2021, 9:46 AM
This revision was automatically updated to reflect the committed changes.

Hi,

We've seen a crash with this patch.
Running

opt -o /dev/null bbi-53235.ll -loop-vectorize

yields

opt: ../lib/Transforms/Vectorize/VPlan.cpp:198: llvm::Value *llvm::VPTransformState::get(llvm::VPValue *, const llvm::VPIteration &): Assertion `hasVectorValue(Def, Instance.Part)' failed.

fhahn added a comment.Feb 22 2021, 8:14 AM

Hi,

We've seen a crash with this patch.
Running

opt -o /dev/null bbi-53235.ll -loop-vectorize

yields

opt: ../lib/Transforms/Vectorize/VPlan.cpp:198: llvm::Value *llvm::VPTransformState::get(llvm::VPValue *, const llvm::VPIteration &): Assertion `hasVectorValue(Def, Instance.Part)' failed.

Thanks for the heads-up. Should be fixed by c7ee57f1dccf

Hi,

We've seen a crash with this patch.
Running

opt -o /dev/null bbi-53235.ll -loop-vectorize

yields

opt: ../lib/Transforms/Vectorize/VPlan.cpp:198: llvm::Value *llvm::VPTransformState::get(llvm::VPValue *, const llvm::VPIteration &): Assertion `hasVectorValue(Def, Instance.Part)' failed.

Thanks for the heads-up. Should be fixed by c7ee57f1dccf

Yes, thanks!