Page MenuHomePhabricator

[TTI] Add optional VecPred argument to getCmpSelInstrCost.
ClosedPublic

Authored by fhahn on Oct 23 2020, 12:50 PM.

Details

Summary

On some targets, like AArch64, vector selects can be efficiently lowered
if the vector condition is a compare with a supported predicate.

This patch adds a new argument to getCmpSelInstrCost, to indicate the
predicate of the feeding select condition. Note that it is not
sufficient to use the context instruction when querying the cost of a
vector select starting from a scalar one, because the condition of the
vector select could be composed of compares with different predicates.

This change greatly improves modeling the costs of certain
compare/select patterns on AArch64.

I am also planning on putting up patches to make use of the new argument in
SLPVectorizer & LV.

Diff Detail

Event Timeline

fhahn created this revision.Oct 23 2020, 12:50 PM
fhahn requested review of this revision.Oct 23 2020, 12:50 PM
fhahn added a comment.Oct 24 2020, 1:04 PM

Looks like there's already a related bug report https://bugs.llvm.org/show_bug.cgi?id=40376. Thanks @RKSimon for mentioning the patch there.

RKSimon added inline comments.Oct 25 2020, 11:20 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
702

Would it make sense to do this in TargetTransformInfo::getCmpSelInstrCost? Most platforms are likely to do something similar.

706

isIntPredicate ?

AArch64 costs sound OK, especially if we can fix NE codegen to be more likely to be a single instruction.

I would personally keep the args in (Opcode, Types, CondCode, CostKind, CtxI) order, to keep them more consistent with the other cost functions. Now that I look at them though I don't think they are as consistent as I believed they were!

llvm/test/Analysis/CostModel/AArch64/vector-select.ll
42–43

Huh.

fhahn updated this revision to Diff 300664.Oct 26 2020, 7:26 AM
fhahn marked an inline comment as done.

AArch64 costs sound OK, especially if we can fix NE codegen to be more likely to be a single instruction.

I would personally keep the args in (Opcode, Types, CondCode, CostKind, CtxI) order, to keep them more consistent with the other cost functions. Now that I look at them though I don't think they are as consistent as I believed they were!

I think having the CondCode after the types would make sense. One advantage of the current ordering is that we do not need to update all existing callers that pass in CtxI. That convenient initially, but there are not too many users of the function that would need updating. Happy to update to whatever order is preferred before landing the patch (threading things through properly everywhere is a bit of a pain, so I'd prefer updating once everyone is happy, to just do it once).

fhahn added a comment.Oct 26 2020, 7:26 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
702

I think that makes sense, but unfortunately it seems like TargetTransformInfo::getCmpSelInstrCost is not called for at least -analyze -costmodel, because it uses TargetTransformInfoImplCRTPBase::getUserCost, which delegates directly to the target-specific getCmpSelInstrCost. Not sure if there's a convenient way to work around that?

dmgreen accepted this revision.Oct 28 2020, 12:19 AM

I would put it after the type, but other than that this LGTM

This revision is now accepted and ready to land.Oct 28 2020, 12:19 AM
RKSimon accepted this revision.Oct 28 2020, 3:47 AM

Yeah, the TTI/CRTP layout makes code reuse a pain in some places - thanks for trying anyway. LGTM

fhahn updated this revision to Diff 301713.Thu, Oct 29, 12:39 PM

I would put it after the type, but other than that this LGTM

I updated the patch to pass VecPred before CostKind, which also makes it non-optional. That wasn't too bad and now all the places that would be good to update are more visible. I've the patch for SLP up already and will also update LV. For some of the other users I am not too sure though.

fhahn retitled this revision from [CostModel] Add optional VecPred argument to getCmpSelInstrCost. to [TTI] Add optional VecPred argument to getCmpSelInstrCost..Fri, Oct 30, 6:46 AM
fhahn updated this revision to Diff 301884.Fri, Oct 30, 6:47 AM

Rebased after recent changes to SLPVectorizer

This revision was landed with ongoing or failed builds.Fri, Oct 30, 7:04 AM
This revision was automatically updated to reflect the committed changes.
pcc added a subscriber: pcc.Fri, Oct 30, 2:07 PM
pcc added inline comments.
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
958

This fails an assertion if the instruction is a constant. If you have a copy of the Android NDK you should be able to reproduce using the in-tree HWASan runtime with something like:

/path/to/build-directory/bin/clang++ -o hwasan_dynamic_shadow.o -c compiler-rt/lib/hwasan/hwasan_dynamic_shadow.cpp -DHWASAN_WITH_INTERCEPTORS=1 -Icompiler-rt/lib --target=aarch64-linux-android29 --sysroot=/path/to/android-ndk-r21/toolchains/llvm/prebuilt/linux-x86_64/sysroot -B/path/to/android-ndk-r21/toolchains/llvm/prebuilt/linux-x86_64 -O3 -fdiagnostics-color -Wall -Wextra -Wno-unused-parameter -Wdelete-non-virtual-dtor -Wstring-conversion -no-canonical-prefixes -Werror=date-time -Wcovered-switch-default -fPIC -funwind-tables -gline-tables-only -fvisibility=hidden -std=c++14 -fvisibility-inlines-hidden -fno-exceptions -fno-rtti

But let me know if you need a repro.

vitalybuka added inline comments.
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
958

same crash on linux bot without android http://lab.llvm.org:8011/#/builders/85/builds/383

fhahn added a comment.Fri, Oct 30, 2:26 PM

Interesting! O reverted the patch for now while I investigate.