Page MenuHomePhabricator

[WIP][SimplifyCFG] Replace FoldTwoEntryPHINode i1 hack with a different hack.
Needs ReviewPublic

Authored by efriedma on Jul 22 2021, 1:47 PM.

Details

Summary

Both of these hacks are designed to handle @test9 in llvm/test/Transforms/SimplifyCFG/switch_create.ll (and a duplicate copy of the test in switch_create-custom-dl.ll). There are side-effects on a couple other tests, but nothing really significant, as far as I can tell, except for two-entry-phi-node.ll.

The new test two-entry-phi-node.ll seems to test i1 heuristic itself, but doesn't really give any hint why we want that heuristic.

I'd appreciate any feedback if this makes sense, or if anyone has testcases that showcase the practical effects of the heuristic. I have an internal testcase where killing off the i1 heuristic helps, but it's not in a state where I can post it.

(See also discussion at https://reviews.llvm.org/rG5419b671375c)

Diff Detail

Unit TestsFailed

TimeTest
2,710 msx64 debian > libarcher.critical::critical.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/critical/critical.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/critical/critical.c
2,660 msx64 debian > libarcher.critical::lock-nested.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/critical/lock-nested.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/lock-nested.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/lock-nested.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/lock-nested.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/critical/lock-nested.c
2,830 msx64 debian > libarcher.parallel::parallel-firstprivate.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-firstprivate.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-firstprivate.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-firstprivate.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-firstprivate.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-firstprivate.c
2,760 msx64 debian > libarcher.parallel::parallel-simple.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-simple.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple.c.tmp -latomic && env OMP_TOOL_VERBOSE_INIT=stderr env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple.c.tmp.log 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-simple.c --check-prefixes CHECK,TSAN_ON
2,780 msx64 debian > libarcher.parallel::parallel-simple2.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-simple2.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple2.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple2.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/parallel/Output/parallel-simple2.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/parallel/parallel-simple2.c
View Full Test Results (20 Failed)

Event Timeline

efriedma created this revision.Jul 22 2021, 1:47 PM
efriedma requested review of this revision.Jul 22 2021, 1:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2021, 1:47 PM

I like this, it's far less fragile,
but as https://bugs.llvm.org/show_bug.cgi?id=51149 reports,
there are some bad performance concerns with FoldTwoEntryPHINode,
and in fact i just tuned/fixed heuristic in rG7ef6f019090f3979fa345105b9ac95ac589c6cf9
to workaround them.

Presumably FoldTwoEntryPHINode is missing some other profitability check.
Not sure about plain build, but we could at least try to use branch weights.

As far as I can tell, there's nothing specifically wrong with flattening i1 values. Maybe we're slightly underestimating the cost of some of these i1 logic ops on x86? The more general issue is some combination of flattening the CFG too aggressively, and not unflattening aggressively enough before SelectionDAG, I think. Unfortunately, that's a delicate area to mess with; any change will inevitably break something.

As far as I can tell, there's nothing specifically wrong with flattening i1 values. Maybe we're slightly underestimating the cost of some of these i1 logic ops on x86? The more general issue is some combination of flattening the CFG too aggressively, and not unflattening aggressively enough before SelectionDAG, I think. Unfortunately, that's a delicate area to mess with; any change will inevitably break something.

I agree with everything said above.

I guess, then, the question is what order we want to do things in. Do we land this essentially as-is, then try to come up with a more reliable way to handle the cases that fall out? Or do we put this on hold until we have improvements to cost estimation and/or CFG unflattening?

I guess, then, the question is what order we want to do things in. Do we land this essentially as-is, then try to come up with a more reliable way to handle the cases that fall out? Or do we put this on hold until we have improvements to cost estimation and/or CFG unflattening?

If this goes in as-is right now, @wmi will re-raise https://bugs.llvm.org/show_bug.cgi?id=51149
If that's fine, then let's do it, else i guess we need to try to do some legwork first.
Let me see if i can add some branch weights checks to these transforms first.

Since we'll probably be unfortunate enough to have to deal with perf regressions anyways,
my plan is to merge FoldTwoEntryPHINode and SpeculativelyExecuteBB enhancing their collective functionality
and dropping the hacks.