Page MenuHomePhabricator

[X86][CostModel] Strip bitcasts when looking for store GEP operand
AbandonedPublic

Authored by aeubanks on Apr 1 2022, 11:21 AM.

Details

Reviewers
RKSimon
ABataev
Summary

This heuristic was introduced in D35888. Assuming it is correct, this
patch should make cost modeling a bit more robust.

This came about when looking at opaque pointers where a bitcast that previously appeared hid this heuristic. This actually regresses the benchmark I was looking at due to less loop unrolling (gesummv in llvm-test-suite), but I'm not sure how else to solve that besides removing this heuristic entirely.

Diff Detail

Unit TestsFailed

TimeTest
60,040 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir ; /usr/bin/cmake --build . --target check-standalone | tee /var/lib/buildkite-agent/builds/llvm-project/build/tools/mlir/test/Examples/standalone/Output/test.toy.tmp | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/mlir/test/Examples/standalone/test.toy
60,040 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

aeubanks created this revision.Apr 1 2022, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 11:21 AM
aeubanks requested review of this revision.Apr 1 2022, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 11:21 AM
aeubanks edited the summary of this revision. (Show Details)Apr 1 2022, 11:24 AM
aeubanks added a reviewer: RKSimon.
RKSimon added inline comments.Apr 7 2022, 3:54 AM
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
3910

I wonder whether this needs inverting - and we default stores to a cost of TTI::TCC_Basic * 2 UNLESS after stripping we find all indices are constant?

nikic added a subscriber: nikic.Apr 7 2022, 5:47 AM
nikic added inline comments.
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
3910

I believe this code is intended to catch the case where we have a GEP that was modeled as free due to addressing mode support, but has non-constant indices and that addressing mode is non-free in store instructions. So I think the current logic is right, though could be more precise if we checked that the GEP cost is actually TCC_Free, otherwise we might double-cost the GEP.

aeubanks abandoned this revision.Wed, Jul 27, 8:17 AM