Page MenuHomePhabricator

[VPlan] Use VPDef for VPInterleaveRecipe.
ClosedPublic

Authored by fhahn on Nov 1 2020, 10:45 AM.

Details

Summary

This patch turns updates VPInterleaveRecipe to manage the values it defines
using VPDef. The VPValue is used during VPlan construction and
codegeneration instead of the plain IR reference where possible.

Diff Detail

Event Timeline

fhahn created this revision.Nov 1 2020, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2020, 10:45 AM
fhahn requested review of this revision.Nov 1 2020, 10:45 AM
fhahn updated this revision to Diff 303448.Nov 6 2020, 7:21 AM

Update after VPDef does not inherit from VPUser directly.

fhahn retitled this revision from [VPlan] Use VPdef for VPInterleaveRecipe. to [VPlan] Use VPDef for VPInterleaveRecipe..Dec 15 2020, 7:45 AM
gilr added inline comments.Dec 16 2020, 12:49 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8387

Could you elaborate on why applying the interleave groups is moved back to VPlan creation?

fhahn added inline comments.Dec 16 2020, 1:14 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8387

The current handling (create recipes for each instruction and remove & replace them with a single interleave recipe later) seems like a work-around and now that we can manage the VPValues for the members of the interleave groups directly, I think it makes sense to directly create them along with the other recipes.

Unfortunately this requires a bit more extra logic to skip interleave groups members when recording the sink-after points, which is not great. But I think this will go away in the future, if we start by building an initial VPlan and gradually lower to specialized recipes.

But I can also keep the VPInterleaveRecipe creation at the original position, if you prefer.

gilr added inline comments.Dec 16 2020, 5:23 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8387

Unfortunately this requires a bit more extra logic to skip interleave groups members when recording the sink-after points, which is not great.

Right. This dependence between sink-after and interleave groups which translates to careful bookkeeping is why LV applies them as ordered VPlan-VPlan transformations. Ayal presented this change at the 2017 Dev. Meeting as a motivating example for the general direction of an initial trivial VPlan gradually & immediately transformed as decisions are taken, replacing LV's decision-recording maps which make it difficult to compose decisions.

fhahn added inline comments.Dec 16 2020, 5:41 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8387

I have a few patches to get us roughly there and D90712 applies sink-after before lowering to specialized recipes, which makes the recording of recipes redundant. So my preference would be to keep thing as-is in the current patch. But as I said I am more than happy to update the patch to do it the 'old' way for now if you think that's preferable.

gilr added inline comments.Dec 20 2020, 1:57 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8387

But as I said I am more than happy to update the patch to do it the 'old' way for now if you think that's preferable.

I think so, to avoid the bookkeeping and keep the patch's scope minimal.

fhahn updated this revision to Diff 312975.Dec 20 2020, 6:02 AM

Thanks Gil! I restored the original code for creating/inserting VPInterleaveRecipe, to reduce diff.

fhahn added inline comments.Dec 20 2020, 6:18 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8387

Done, its much cleaner now :)

gilr accepted this revision.Dec 20 2020, 10:38 PM

LGTM!

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

Indeed :), Tx Florian!

This revision is now accepted and ready to land.Dec 20 2020, 10:38 PM
This revision was landed with ongoing or failed builds.Dec 21 2020, 3:02 AM
This revision was automatically updated to reflect the committed changes.

This commit is causing the following

int example(int n, int *arr)
{
 int ret = 0;

 for( int i = 0; i < n; i += 2 ) {
  ret = arr[i+1] + arr[i] + ret;
 }

 return ret;
}

to hit an assertion failure when compiled with --target=arm-none-eabi -march=armv8.1-m.main+mve -O2:

llvm-project/llvm/lib/Transforms/Vectorize/VPlan.h:1770: llvm::VPValue* llvm::VPlan::getVPValue(llvm::Value*): Assertion `Value2VPValue.count(V) && "Value does not exist in VPlan"' failed.

with the backtrace showing that the assert was hit in llvm::LoopVectorizationPlanner::adjustRecipesForInLoopReductions.

fhahn added a comment.Jan 5 2021, 7:07 AM

This commit is causing the following

int example(int n, int *arr)
{
 int ret = 0;

 for( int i = 0; i < n; i += 2 ) {
  ret = arr[i+1] + arr[i] + ret;
 }

 return ret;
}

to hit an assertion failure when compiled with --target=arm-none-eabi -march=armv8.1-m.main+mve -O2:

llvm-project/llvm/lib/Transforms/Vectorize/VPlan.h:1770: llvm::VPValue* llvm::VPlan::getVPValue(llvm::Value*): Assertion `Value2VPValue.count(V) && "Value does not exist in VPlan"' failed.

with the backtrace showing that the assert was hit in llvm::LoopVectorizationPlanner::adjustRecipesForInLoopReductions.

Thanks for the report! Should be fixed in 8a47e6252ad43a2eb3238f9b36394571ba13f4a9