Page MenuHomePhabricator

[LV] Prevent vectorization with unsupported element types.
AcceptedPublic

Authored by kmclaughlin on May 11 2021, 9:53 AM.

Details

Summary

This patch adds a TTI function, isElementTypeLegalForScalableVector, to query
whether it is possible to vectorize a given element type. This is called by
isLegalToVectorizeInstTypesForScalable to reject scalable vectorization if
any of the instruction types in the loop are unsupported, e.g:

int foo(__int128_t* ptr, int N)
  #pragma clang loop vectorize_width(4, scalable)
  for (int i=0; i<N; ++i)
    ptr[i] = ptr[i] + 42;

This example currently crashes if we attempt to vectorize since i128 is not a
supported type for scalable vectorization.

Diff Detail

Event Timeline

kmclaughlin created this revision.May 11 2021, 9:53 AM
kmclaughlin requested review of this revision.May 11 2021, 9:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 9:53 AM
sdesmalen added inline comments.May 11 2021, 2:06 PM
llvm/include/llvm/Analysis/TargetTransformInfo.h
1324

Should this instead be changed to isLegalToVectorizeElementType? (that would match the comment at least)

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1643–1644

is this needed? specifically, this function is never called if Ty->isVoidTy

1646

nit: unnecessary braces

sdesmalen retitled this revision from [AArch64][LoopVectorize] Add AArch64TTIImpl::isLegalToVectorizeType function to [LV] Prevent vectorization with unsupported element types..May 11 2021, 2:09 PM
sdesmalen edited the summary of this revision. (Show Details)

@kmclaughlin I hope you don't mind me retitling your patch. I wanted to clarify that this patch and the new interface is not specific to AArch64/SVE.

craig.topper added inline comments.May 11 2021, 5:07 PM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1646

Does this change the behavior of fixed length vectorization of unsupported types when SVE is enabled even when SVE isn't being used for fixed vectors?

david-arm added inline comments.May 12 2021, 1:57 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1646

It looks like it - I wonder if it's possible to delay the check for legal types until we're dealing with a VF, and then we can pass the VF as an extra parameter to the function?

Hello. I thought that the idea was going to be that the costmodel returned an invalid cost for types/operations that could not be handled. And because it had an invalid cost, the vectorizer would then not vectorize with scalable factors? Is that not still the idea?

kmclaughlin marked 4 inline comments as done.
kmclaughlin edited the summary of this revision. (Show Details)
  • Moved the changes which add isLegalToVectorizeElementType into a seperate patch (D102515)
  • Added canVectorizeInstructionTypes, which iterates through all instructions in the loop and queries whether it is legal to vectorize the element type.
  • Call canVectorizeInstructionTypes from getMaxLegalScalableVF to ensure we reject scalable vectorization if the loop contains any unsupported types.

Hello. I thought that the idea was going to be that the costmodel returned an invalid cost for types/operations that could not be handled. And because it had an invalid cost, the vectorizer would then not vectorize with scalable factors? Is that not still the idea?

Hi @dmgreen, thanks for taking a look at this! We should be returning invalid costs for operations that are not supported, though I think the idea is that we should also be trying to reject scalable vectorisation early in situations such as the one here so that we don't have to rely on what the cost model is returning. As we can also only cost operations that are legal to vectorise, if we get an invalid cost for an instruction that we considered legal, this suggests either the cost model is incomplete or legalisation was not strict enough.
I've added D102515 to ensure we are returning an invalid cost for memory ops with unsupported types.

Matt added a subscriber: Matt.May 14 2021, 12:13 PM
joechrisellis added inline comments.May 17 2021, 1:43 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1523–1533

I think this is equivalent to:

bool canVectorizeInstructionTypes(Loop *TheLoop, ElementCount VF) {
  for (BasicBlock *BB : TheLoop->blocks())
    for (Instruction &I : BB->instructionsWithoutDebug()) {
      auto *Ty = I.getType();
      if (!Ty->isVoidTy() &&
          !TTI.isLegalToVectorizeElementType(Ty, VF.isScalable()))
        return false;
    }
  return true;
}

Has the nice side effect of getting rid of the double negative. 🙂

llvm/test/Transforms/LoopVectorize/sve-illegal-type.ll
1 ↗(On Diff #345506)

nit: I usually see the triple being set with:

target triple = "aarch64-linux-gnu"

to simplify the RUN line. IMO in general it makes it a little nicer to run the test as a standalone thing rather than through llvm-lit. 🙂

david-arm added inline comments.May 17 2021, 1:48 AM
llvm/test/Transforms/LoopVectorize/sve-illegal-type.ll
1 ↗(On Diff #345506)

I think many/most of our tests in llvm/test/Transforms/LoopVectorize/AArch64/ do it this way, i.e. specify the triple on the command line so @kmclaughlin's RUN line is more the norm here. But either way works!

@kmclaughlin - since this is AArch64 specific can you move this to the AArch64 directory?

david-arm added inline comments.May 17 2021, 1:51 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1523

I don't think we need to pass TheLoop in here, since I think it's a member of the class already?

kmclaughlin marked 4 inline comments as done.

Addressing review comments from @joechrisellis & @david-arm:

  • Removed IllegalTy from canVectorizeInstructionTypes() & stopped passing TheLoop to the function
  • Moved the test file to the AArch64 directory & removed the triple from its RUN line
david-arm accepted this revision.May 18 2021, 6:12 AM

LGTM! Thanks for dealing with review comments!

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

nit: Could you add a simple comment above the function before committing? Something like

/// Returns true if all types found in the loop are legal to vectorize.

maybe?

llvm/test/Transforms/LoopVectorize/AArch64/sve-illegal-type.ll
2

nit: Since this test is now in the AArch64 directory I think you can replace "-force-target-supports.." with "-mattr=+sve"?

This revision is now accepted and ready to land.May 18 2021, 6:12 AM
fhahn added a subscriber: fhahn.May 18 2021, 6:15 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1523

I think terminology used in other places is 'widening' instead of 'vectorizing' , both in the cost model and codegen. Does vectorizing here means something different? Also would be good to add a comment for the function..

david-arm added inline comments.May 18 2021, 6:41 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1523

I guess they're the same? I hadn't deliberately chosen the word vectorize over widen to be honest. In this case I think we specifically want to know if the target has hardware support for vector instructions involving a given type because we cannot fall back on scalarisation.

In general, is there a preference to using Widen instead of Vectorize in naming schemes and comments? It's just I do see other functions in this file use the word Vectorize in functions and class names so I guess it seemed natural to create functions with the word Vectorize in them.

fhahn added inline comments.Wed, May 19, 3:15 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1523

I don't think there's a definitive answer. The current usage is not as clear cut as I thought. I was thinking about the terminology used for the various widenInstruction, widenGEP, widenIntOrFpInduction, widenPHIInstruction & co vs. vectorizeInterleaveGroup which does not simplify widen instructions, together with the widening decision terminology used in the cost model.

In any case, a doc-comment would definitely be helpful. Another thing to consider to align the name with TTI.isLegalToVectorizeElementType to something like isLegalTo(Vectorize/Widen)InstructionType

kmclaughlin marked an inline comment as done.
  • Renamed canVectorizeInstructionTypes to isLegalToVectorizeInstTypesForScalable & added a comment above it
  • Replaced -force-target-supports-scalable-vectors with -mattr=+sve in sve-illegal-type.ll
david-arm accepted this revision.Mon, May 24, 8:11 AM

LGTM! Seems like outstanding review comments have been addressed. Thanks!

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

nit: I think perhaps you can just remove the VF argument now?

kmclaughlin marked an inline comment as done.
kmclaughlin edited the summary of this revision. (Show Details)
  • Moved the addition of isElementTypeLegalForScalableVector into this patch from D102515
  • Removed unused VF from isLegalToVectorizeInstTypesForScalable
  • Merged the isElementTypeLegalForScalableVector and isLegalElementTypeForSVE functions. Since isElementTypeLegalForScalableVector does not check the VF anymore, there was no additional benefit to them being separate.
sdesmalen added inline comments.Tue, Jun 8, 9:58 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
1327

Missing doxygen comment.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
295

To reduce the diff, can you just rename isLegalElementTypeForSVE?

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

I'd like to suggest to have a name that's more generic: canWidenLoopWithScalableVectors, so that it can be reused for other purposes. D102394 and subsequently D101916 do that already. If you make a similar change as in D102394 where you move canVectorizeReductions into this function, then @david-arm can rebase his patch when he's back.

kmclaughlin marked 2 inline comments as done.
  • Renamed isLegalElementTypeForSVE to isElementTypeLegalForScalableVector, but kept the implementation in AArch64TargetTransformInfo.h
  • Renamed isLegalToVectorizeInstTypesForScalable to canWidenLoopWithScalableVectors and moved canVectorizeReductions into this function, similar to D102394