Page MenuHomePhabricator

[ScalarEvolution] Skip values from unreachable bbs for phi ranges.
Needs ReviewPublic

Authored by fhahn on Nov 7 2022, 2:17 PM.

Details

Summary

Skip incoming value from unreachable blocks when computing ranges for
phis. Constructing SCEVs for values from unreachable blocks may lead to
violating assumptions based on dominance.

One example is the added LV test case. Here an assertion in GroupByComplexity
is triggered when constructing a SCEV for the incoming value from the unreachable
block because there's no dominance relation between 2 AddRecs.

It removes pr49856.ll because it is already part of
shift-recurrences.ll and needs updating.

Fixes #58811.

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

Event Timeline

fhahn created this revision.Nov 7 2022, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2022, 2:17 PM
fhahn requested review of this revision.Nov 7 2022, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2022, 2:17 PM

The assertion from the bug report seems several levels removed from the top-level APIs; can we add an assertion somewhere a bit closer to the API misuse? I don't see a problem with this patch, exactly, but I'm not sure why the current code is wrong, as opposed to just being slightly suboptimal.

Do you mind giving more details on what assumption exactly was broken?