Page MenuHomePhabricator

Revert "[LoopVectorize] Simplify scalar cost calculation in getInstructionCost."
Needs ReviewPublic

Authored by fhahn on May 13 2022, 3:40 AM.

Details

Summary

This reverts 6998f8ae2d14e096aff33968f226587b5c1a193a.

Unfortunately another issue surfaced where the underlying assumption for
the patch does not hold and we hit the assertion for an Add instruction.

In pr55096-scalarize-add.ll, there's an add that will get scalarized (as
it is an induction update that has scalarized users) and there will be
multiple copies after vectorization.

The way we currently compute the costs for instructions involved in
induction updates is not really accurate, but I don't think there's an
easy way to ensure scalarized instructions will have single copy after
vectorization.

Fixes #55096.

Diff Detail

Unit TestsFailed

TimeTest
60,080 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
2,660 msx64 debian > libFuzzer.libFuzzer::fuzzer-finalstats.test
Script: -- : 'RUN: at line 1'; /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/SimpleTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-finalstats.test.tmp-SimpleTest

Event Timeline

fhahn created this revision.May 13 2022, 3:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2022, 3:40 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn requested review of this revision.May 13 2022, 3:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2022, 3:40 AM

Hi @fhahn, I think I remember when doing this patch these asserts were put in to deal with reviewer requests, but in my original patch I didn't have any asserts at all. I worry that reverting the patch doesn't solve the underlying problem, which is that the cost calculations are sometimes incorrect and difficult to follow. As a compromise could we just remove the assert, but keep the simpler (and easier to understand) logic? This patch has been in the codebase for over a year now without problems - personally it feels more appropriate to fix the assert rather than revert. What do you think?