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

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

Details

Reviewers

sdesmalen
david-arm
Ayal
gilr

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

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	60,080 ms	x64 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 ms	x64 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

fhahn mentioned this in D99718: [LoopVectorize] Simplify scalar cost calculation in getInstructionCost.May 13 2022, 3:42 AM

Harbormaster completed remote builds in B164277: Diff 429187.May 13 2022, 4:47 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?