Page MenuHomePhabricator

[InstCombine] extract(u[add|sub].with.overflow(X, C), 0) --> [add|sub](X, C)
AbandonedPublic

Authored by nico-abram on Apr 30 2022, 4:15 PM.

Details

Summary

Trying to fix https://github.com/llvm/llvm-project/issues/53978 I ran into something like this: https://alive2.llvm.org/ce/z/bduF3e (generalized here https://alive2.llvm.org/ce/z/HdrUXt ) which is not currently optimized.

@nikic mentioned on discord that it might make more sense to canonicalize the intrinsic into a simple add/sub instead of trying to match that specific pattern

Yes, InstCombine would be the place. Though frankly, we should probably just canonicalize uadd/usub away from intrinsics. That should result in better mid-end optimization, and CGP can reform them

This patch does that, but only when the addition is by a constant. I added @uadd_res_ugt_smaller_const_or_ov and @uadd_res_ult_const_five_and_ov as a baseline (The baseline is in the second diff. The third one is the patch against main HEAD).

With this patch, the original case in https://github.com/llvm/llvm-project/issues/53978 gets optimized to a constant false.

Diff Detail

Unit TestsFailed

TimeTest
60,100 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,130 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,020 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.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/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,050 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
60,020 msx64 debian > libFuzzer.libFuzzer::minimize_crash.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/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest
View Full Test Results (6 Failed)

Event Timeline

nico-abram created this revision.Apr 30 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2022, 4:15 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
nico-abram requested review of this revision.Apr 30 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2022, 4:15 PM
nico-abram updated this revision to Diff 426271.
nico-abram edited the summary of this revision. (Show Details)
nikic added a reviewer: spatel.May 1 2022, 1:15 AM

Hrm, unfortunately canonicalizing the intrinsic to add/icmp is not safe with undef: https://alive2.llvm.org/ce/z/cgiQ_n We effectively introduce two uses of undef, which can each take a different value.

This might need to be addressed on the front-end side instead, by not emitting uadd/usub intrinsics in the first place.

ojeda added a subscriber: ojeda.May 1 2022, 1:36 AM
nico-abram abandoned this revision.May 2 2022, 5:56 PM

Abandoning revision since the transformation is not safe.

"front-end" means rustc/clang right?