Page MenuHomePhabricator

[OpenMP] Ensure createXXX functions will always call updateToLocation
AcceptedPublic

Authored by jdoerfert on Jun 9 2022, 4:55 AM.

Details

Summary

It is easy to not set the debug location which will result in problems
in debug builds. Hence, we should always use the helper
updateToLocation for all user facing entry points of the OMPIRBuilder.

Diff Detail

Unit TestsFailed

TimeTest
60,050 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,120 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,090 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
60,070 msx64 debian > libFuzzer.libFuzzer::out-of-process-fuzz.test
Script: -- : 'RUN: at line 2'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp
60,030 msx64 debian > libFuzzer.libFuzzer::value-profile-load.test
Script: -- : 'RUN: at line 2'; /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/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest
View Full Test Results (8 Failed)

Event Timeline

jdoerfert created this revision.Jun 9 2022, 4:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 4:55 AM
jdoerfert requested review of this revision.Jun 9 2022, 4:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 4:55 AM

LG. I have one question. Is there a contract in place where the InsertionPointGuard is used to reset the IP back to the one at entry and would this change break it?

LG. I have one question. Is there a contract in place where the InsertionPointGuard is used to reset the IP back to the one at entry and would this change break it?

I don't think the Builder should reset to entry. It's an implicit contract that is hard to maintain and arguably useless as there is no reason to believe one would insert two "OpenMP things" at exactly the same point. IPs during codegen generally move and we should make all IPs explicit. These guards were introduced in a single patch (IIRC) and should not serve a purpose.

This revision is now accepted and ready to land.Jun 14 2022, 3:55 PM

Why the pattern to just return without doing anything without an insert location? IMHO this should be an error instead of silently ignoring things.

I don't think the Builder should reset to entry. It's an implicit contract that is hard to maintain and arguably useless as there is no reason to believe one would insert two "OpenMP things" at exactly the same point. IPs during codegen generally move and we should make all IPs explicit. These guards were introduced in a single patch (IIRC) and should not serve a purpose.

I completely agree and already encountered problems with the reset location having become invalid between that guard and its dtor.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1846–1848

The insert- and debug location is already set inside createLoopSkeleton, setting it here has no effect.

On line 1849 updateToLocation is then called and it handles the "no insert location" case by not connecting the loop skeleton to anything (as the caller wishes). return null makes this impossible.

Why the pattern to just return without doing anything without an insert location? IMHO this should be an error instead of silently ignoring things.

Because that is the clang pattern and we want/need to be compatible.

I don't think the Builder should reset to entry. It's an implicit contract that is hard to maintain and arguably useless as there is no reason to believe one would insert two "OpenMP things" at exactly the same point. IPs during codegen generally move and we should make all IPs explicit. These guards were introduced in a single patch (IIRC) and should not serve a purpose.

I completely agree and already encountered problems with the reset location having become invalid between that guard and its dtor.

Because that is the clang pattern and we want/need to be compatible.

Clang does not use such a pattern. When IRBuilder creates an instruction without an insert position, it still creates the instruction, but does not insert it into a function. createCanonicalLoop was supposed to do something similar (create the loop, but do not connect it to the CFG).

Because that is the clang pattern and we want/need to be compatible.

Clang does not use such a pattern.

if (!CGF.HaveInsertPoint())                                                                                                                                                                                                                              
   return;

appears 35 times in CGOpenMPRuntime.cpp

if (!CGF.HaveInsertPoint())                                                                                                                                                                                                                              
   return;

appears 35 times in CGOpenMPRuntime.cpp

Commenting them all out still passes all tests. It essentially dead/untested code. Also seen in the code coverage: https://lab.llvm.org/coverage/coverage-reports/coverage/Users/buildslave/jenkins/workspace/coverage/llvm-project/clang/lib/CodeGen/CGOpenMPRuntime.cpp.html