Page MenuHomePhabricator

[SCEV] Serialize function calls in function arguments.
AcceptedPublic

Authored by chapuni on May 15 2022, 5:06 AM.

Details

Summary

Evaluation odering in function call arguments is implementation-dependent.
In fact, gcc evaluates bottom-top and clang does top-bottom.

I confirmed, with this change, that every getSCEV() is called idetically
between clang and gcc in the compilation of the LLVM tree.

Fixes #55283

FIXME: I didn't fix all of the matter.

Diff Detail

Unit TestsFailed

TimeTest
60,090 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,030 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,030 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,020 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

Event Timeline

chapuni created this revision.May 15 2022, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 5:07 AM
chapuni requested review of this revision.May 15 2022, 5:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 5:07 AM
Allen added a subscriber: Allen.May 15 2022, 1:09 PM
nikic accepted this revision.May 16 2022, 12:09 PM

LGTM with some code style suggestions.

llvm/lib/Analysis/ScalarEvolution.cpp
1807

I'd just reassign LHS and RHS here (and below). The LHSz/RHSz names are bit unusual.

3127

const SCEV * here and elsewhere. This auto is not saving a lot of characters :)

7576

Please move these as separate assignments before the if (here and below).

This revision is now accepted and ready to land.May 16 2022, 12:09 PM
chapuni marked an inline comment as done.May 18 2022, 7:25 AM

At the moment, I have committed llvmorg-15-init-10782-g6ca7eb2c6d7d

chapuni added inline comments.May 18 2022, 7:38 AM
llvm/lib/Analysis/ScalarEvolution.cpp
1807

I was afraid since LHS and RHS are captured and assigned by matchURem(const SCEV*, const SCEV*&, const SCEV*&)
I thought they might be dedicated to matchURem().

How about;

  • Rename original LHS,RHS to LHSURem,RHSURem
  • Use LHS,RHS as generic instead of my LHSz,RHSz
7576

I avoided to evaluate RHS if the former condition (LHS) was false.
Could I split conditions? Or evaluate both LHS and RHS before conditions?

LHS =  = getSCEV(U->getOperand(0);
if (isKnownNonNegative(LHS))) {
  RHS = getSCEV(U->getOperand(1);
  if (isKnownNonNegative(RHS)))
    return getUDivExpr(LHS, RHS);
}
nikic added inline comments.Mon, May 30, 4:21 AM
llvm/lib/Analysis/ScalarEvolution.cpp
1807

I don't think the capture here matters. These are just out parameters for matchURem, it's fine to reuse the variables.