MaskRay created this revision. MaskRay added reviewers: davidxl, vsk, xur. Herald added subscribers: cfe-commits, dang, jfb. Herald added a project: clang. MaskRay requested review of this revision.
GCC 7 introduced -fprofile-update={atomic,prefer-atomic} (prefer-atomic is for best efforts (some targets do not support atomics)) to increment counters atomically, which is exactly what we have done with -fprofile-instr-generate (D50867 <https://reviews.llvm.org/D50867>) and -fprofile-arcs (b5ef137c11b1cc6ae839ee75b49233825772bdd0 <https://reviews.llvm.org/rGb5ef137c11b1cc6ae839ee75b49233825772bdd0>). This patch adds the option to clang to surface the internal options at driver level. GCC 7 also turned on -fprofile-update=prefer-atomic when -pthread is specified, but it has performance regression (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89307). So we don't follow suit. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D87737 Files: clang/include/clang/Basic/CodeGenOptions.def clang/include/clang/Driver/Options.td clang/lib/CodeGen/BackendUtil.cpp clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Frontend/CompilerInvocation.cpp clang/test/CodeGen/code-coverage-tsan.c clang/test/CodeGen/tsan-instrprof-atomic.c clang/test/Driver/fprofile-update.c
Index: clang/test/Driver/fprofile-update.c =================================================================== --- /dev/null +++ clang/test/Driver/fprofile-update.c @@ -0,0 +1,15 @@ +/// For -fprofile-instr-generate and -fprofile-arcs, increment counters atomically +/// if -fprofile-update={atomic,prefer-atomic} or -fsanitize=thread is specified. +// RUN: %clang -### %s -c -target x86_64-linux -fsanitize=thread %s 2>&1 | FileCheck %s +// RUN: %clang -### %s -c -fprofile-update=atomic 2>&1 | FileCheck %s +// RUN: %clang -### %s -c -fprofile-update=prefer-atomic 2>&1 | FileCheck %s + +// CHECK: "-fprofile-update=atomic" + +// RUN: %clang -### %s -c -fprofile-update=atomic -fprofile-update=single 2>&1 | FileCheck %s --check-prefix=SINGLE + +// SINGLE-NOT: "-fprofile-update=atomic" + +// RUN: not %clang %s -c -fprofile-update=unknown 2>&1 | FileCheck %s --check-prefix=ERROR + +// ERROR: error: unsupported argument 'unknown' to option 'fprofile-update=' Index: clang/test/CodeGen/tsan-instrprof-atomic.c =================================================================== --- clang/test/CodeGen/tsan-instrprof-atomic.c +++ clang/test/CodeGen/tsan-instrprof-atomic.c @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 %s -emit-llvm -fprofile-instrument=clang -fsanitize=thread -o - | FileCheck %s +// RUN: %clang_cc1 %s -emit-llvm -fprofile-instrument=clang -fprofile-update=atomic -o - | FileCheck %s // CHECK: define {{.*}}@foo // CHECK-NOT: load {{.*}}foo Index: clang/test/CodeGen/code-coverage-tsan.c =================================================================== --- clang/test/CodeGen/code-coverage-tsan.c +++ clang/test/CodeGen/code-coverage-tsan.c @@ -1,11 +1,12 @@ -/// -fsanitize=thread requires the (potentially concurrent) counter updates to be atomic. -// RUN: %clang_cc1 %s -triple x86_64 -emit-llvm -fsanitize=thread -femit-coverage-notes -femit-coverage-data \ +/// -fprofile-update=atmomic (implied by -fsanitize=thread) requires the +/// (potentially concurrent) counter updates to be atomic. +// RUN: %clang_cc1 %s -triple x86_64 -emit-llvm -fprofile-update=atomic -femit-coverage-notes -femit-coverage-data \ // RUN: -coverage-notes-file /dev/null -coverage-data-file /dev/null -o - | FileCheck %s // CHECK-LABEL: void @foo() /// Two counters are incremented by __tsan_atomic64_fetch_add. -// CHECK: call i64 @__tsan_atomic64_fetch_add -// CHECK-NEXT: call i32 @__tsan_atomic32_fetch_sub +// CHECK: atomicrmw add i64* {{.*}} @__llvm_gcov_ctr +// CHECK-NEXT: atomicrmw sub i32* _Atomic(int) cnt; void foo() { cnt--; } Index: clang/lib/Frontend/CompilerInvocation.cpp =================================================================== --- clang/lib/Frontend/CompilerInvocation.cpp +++ clang/lib/Frontend/CompilerInvocation.cpp @@ -884,6 +884,7 @@ Opts.DebugRangesBaseAddress = Args.hasArg(OPT_fdebug_ranges_base_address); setPGOInstrumentor(Opts, Args, Diags); + Opts.AtomicProfileUpdate = Args.hasArg(OPT_fprofile_update_EQ); Opts.InstrProfileOutput = std::string(Args.getLastArgValue(OPT_fprofile_instrument_path_EQ)); Opts.ProfileInstrumentUsePath = Index: clang/lib/Driver/ToolChains/Clang.cpp =================================================================== --- clang/lib/Driver/ToolChains/Clang.cpp +++ clang/lib/Driver/ToolChains/Clang.cpp @@ -868,6 +868,17 @@ CmdArgs.push_back(Args.MakeArgString(Twine("-fprofile-filter-files=" + v))); } + if (const auto *A = Args.getLastArg(options::OPT_fprofile_update_EQ)) { + StringRef Val = A->getValue(); + if (Val == "atomic" || Val == "prefer-atomic") + CmdArgs.push_back("-fprofile-update=atomic"); + else if (Val != "single") + D.Diag(diag::err_drv_unsupported_option_argument) + << A->getOption().getName() << Val; + } else if (TC.getSanitizerArgs().needsTsanRt()) { + CmdArgs.push_back("-fprofile-update=atomic"); + } + // Leave -fprofile-dir= an unused argument unless .gcda emission is // enabled. To be polite, with '-fprofile-arcs -fno-profile-arcs' consider // the flag used. There is no -fno-profile-dir, so the user has no Index: clang/lib/CodeGen/BackendUtil.cpp =================================================================== --- clang/lib/CodeGen/BackendUtil.cpp +++ clang/lib/CodeGen/BackendUtil.cpp @@ -570,7 +570,7 @@ Options.NoRedZone = CodeGenOpts.DisableRedZone; Options.Filter = CodeGenOpts.ProfileFilterFiles; Options.Exclude = CodeGenOpts.ProfileExcludeFiles; - Options.Atomic = LangOpts.Sanitize.has(SanitizerKind::Thread); + Options.Atomic = CodeGenOpts.AtomicProfileUpdate; return Options; } @@ -582,10 +582,7 @@ InstrProfOptions Options; Options.NoRedZone = CodeGenOpts.DisableRedZone; Options.InstrProfileOutput = CodeGenOpts.InstrProfileOutput; - - // TODO: Surface the option to emit atomic profile counter increments at - // the driver level. - Options.Atomic = LangOpts.Sanitize.has(SanitizerKind::Thread); + Options.Atomic = CodeGenOpts.AtomicProfileUpdate; return Options; } Index: clang/include/clang/Driver/Options.td =================================================================== --- clang/include/clang/Driver/Options.td +++ clang/include/clang/Driver/Options.td @@ -853,6 +853,9 @@ def fprofile_exclude_files_EQ : Joined<["-"], "fprofile-exclude-files=">, Group<f_Group>, Flags<[CC1Option, CoreOption]>, HelpText<"Instrument only functions from files where names don't match all the regexes separated by a semi-colon">; +def fprofile_update_EQ : Joined<["-"], "fprofile-update=">, + Group<f_Group>, Flags<[CC1Option, CoreOption]>, Values<"atomic,prefer-atomic,single">, + HelpText<"Set update method of profile counters">; def forder_file_instrumentation : Flag<["-"], "forder-file-instrumentation">, Group<f_Group>, Flags<[CC1Option, CoreOption]>, HelpText<"Generate instrumented code to collect order file into default.profraw file (overridden by '=' form of option or LLVM_PROFILE_FILE env var)">; Index: clang/include/clang/Basic/CodeGenOptions.def =================================================================== --- clang/include/clang/Basic/CodeGenOptions.def +++ clang/include/clang/Basic/CodeGenOptions.def @@ -179,6 +179,7 @@ VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified. VALUE_CODEGENOPT(OptimizeSize, 2, 0) ///< If -Os (==1) or -Oz (==2) is specified. +CODEGENOPT(AtomicProfileUpdate , 1, 0) ///< Set -fprofile-update=atomic /// Choose profile instrumenation kind or no instrumentation. ENUM_CODEGENOPT(ProfileInstr, ProfileInstrKind, 2, ProfileNone) /// Choose profile kind for PGO use compilation.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits