This is an archive of the discontinued LLVM Phabricator instance.

[LV] Avoid scalable vectorization for loops containing alloca
ClosedPublic

Authored by kmclaughlin on Jul 12 2021, 9:09 AM.

Details

Summary

This patch returns an Invalid cost from getInstructionCost() for alloca
instructions if the VF is scalable, as otherwise loops which contain
these instructions will crash when attempting to scalarize the alloca.

Diff Detail

Event Timeline

kmclaughlin created this revision.Jul 12 2021, 9:09 AM
kmclaughlin requested review of this revision.Jul 12 2021, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2021, 9:09 AM
sdesmalen added inline comments.Jul 12 2021, 9:19 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7907

Can you add a comment describing why we don't want to vectorize this for scalable vectors?

llvm/test/Transforms/LoopVectorize/AArch64/scalable-alloca.ll
30

Can you force the loop to be vectorized with a scalable VF using a loop-hint? With the current RUN line and without hints, the loop may make the decision for other reasons (like considering a fixed-VF cheaper, rather than outright ignoring a scalable VF that was otherwise forced, but had an invalid cost).

It may also be worth rebasing your patch on D105806 and adding a CHECK line for one of the remarks (although the remarks may still change depending on review feedback)

david-arm added inline comments.Jul 12 2021, 9:29 AM
llvm/test/Transforms/LoopVectorize/AArch64/scalable-alloca.ll
33

Do we need this alloca and the call to @foo at the bottom? I wonder if it may be simpler to pass a i32** %vla pointer to the function? I think the main thing we care about is the alloca in the loop I think?

34

nit: I think we can simplify the test here by just branching directly, i.e.

br label %for.body

without the icmp.

kmclaughlin marked 4 inline comments as done.

Changes to scalable-alloca.ll:

  • Added a loop hint to force vectorization with a scalable VF
  • Rebased the patch and added CHECK lines for Invalid costs found in the loop (from D105806)
  • Passed i32** %vla to the @alloca function
llvm/test/Transforms/LoopVectorize/AArch64/scalable-alloca.ll
2

Makes sense to use the python file to create the check lines here?

sdesmalen added inline comments.Jul 15 2021, 4:44 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7909

nit: this line is redundant, because the code says as much.

llvm/test/Transforms/LoopVectorize/AArch64/scalable-alloca.ll
2

@CarolineConcatto I don't think that is necessary for this specific patch because this test is not guarding the fixed-width vectorization capabilities of the LV, nor the AArch64 CostModel.

4

Can you also add: CHECK-REMARKS: UserVF ignored because of invalid costs.

9

I'd suggest to remove these CHECK lines, because the fact that this loop vectorizes with fixed-width vectors is down to the cost-model, which means we may have to regenerate the check lines at some point, even though they're not needed for what it is that you're trying to test.

kmclaughlin marked 3 inline comments as done.
  • Removed unnecessary CHECK lines from scalable-alloca.ll
  • Added a check for the "UserVF ignored because of invalid costs" remark
sdesmalen accepted this revision.Jul 15 2021, 7:27 AM

LGTM, thanks.

Just FYI, you'll want to wait with landing it until I've relanded D105806.

This revision is now accepted and ready to land.Jul 15 2021, 7:27 AM
This revision was landed with ongoing or failed builds.Jul 16 2021, 3:48 AM
This revision was automatically updated to reflect the committed changes.