Page MenuHomePhabricator

[SLP] AdjustExtractsCost - remove redundant subvector extraction code
Changes PlannedPublic

Authored by RKSimon on May 2 2022, 6:33 AM.

Details

Summary

I noticed this while investigating replacing the getVectorInstrCost(Instruction::ExtractElement) calls with getScalarizationOverhead instead.

This code should be helping improve the costs of extracting an element from subvectors created during legalization, but removing it seems to have no effect - I'm guessing the getVectorInstrCost/getScalarizationOverhead improvements may have made it redundant?

Diff Detail

Unit TestsFailed

TimeTest
60,060 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp
60,060 msx64 debian > libFuzzer.libFuzzer::large.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LargeTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/large.test.tmp-LargeTest

Event Timeline

RKSimon created this revision.May 2 2022, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 6:33 AM
RKSimon requested review of this revision.May 2 2022, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 6:33 AM
ABataev added a comment.EditedMay 2 2022, 6:48 AM

I rather doubt that it is redundant. This code is for the case, where the buildvector of extractelements results in the different vector size rather than the original size of vector(say, we extract elements from <4x i32> but the the resulting vector is <8xi32> or <2 x i32>). In this case we need to take the inser/extract subvector cost, especially if we insert somewhere in the middle of the vector register. Instead, we need to adjust/fix the idx for TargetTransformInfo::SK_InsertSubvector shuffle kind, currently it is too optimistic. I used 0 here because TTI cost model was not optimal and in many cases we ended up with scalarization overhead rather than actual shuffles.

OK, redundant probably isn't correct - but we still have the situation that removing this code has no effect on tests any more.

It appears to have been added in D99980.

OK, redundant probably isn't correct - but we still have the situation that removing this code has no effect on tests any more.

It appears to have been added in D99980.

Its just the tests we have are not affected anymore but some real code is still affected. The test codebase is not full, many cases are missing. Removing this code may cause ineffective vectorization in some corner cases. We need to keep this and fix

Cost += TTIRef.getShuffleCost(TargetTransformInfo::SK_InsertSubvector,
                                      VecTy, None, 0, EEVTy); <---- 0 is not very correct (too optimistic).

instead.

vporpo added a comment.May 2 2022, 1:32 PM

Somewhat related to this patch: For cost modeling changes it would probably be helpful to have tests with cost values in them. Because some of these changes may not directly affect the generated IR, but would show up in the cost diff. For example something like: https://reviews.llvm.org/D124802 . Any thoughts?

RKSimon planned changes to this revision.May 7 2022, 9:19 AM

I'll come back to this - ideally we'd avoid handling the subvector cost adjustments in SLP and let getScalarizationOverhead deal with it.

I'll come back to this - ideally we'd avoid handling the subvector cost adjustments in SLP and let getScalarizationOverhead deal with it.

I think this is possible, but need to reimplement the whole AdjustExtractsCost, need to use getScalarizationOverhead instead of getVectorInstrCostr.

I'll come back to this - ideally we'd avoid handling the subvector cost adjustments in SLP and let getScalarizationOverhead deal with it.

I think this is possible, but need to reimplement the whole AdjustExtractsCost, need to use getScalarizationOverhead instead of getVectorInstrCostr.

D125527 improves the duplicated subvector extraction costs on x86