[PATCH] D34796: upporting -f(no)-reorder-functions flag, clang side change

davidxl added a comment. The patch is missing documentation change. https://reviews.llvm.org/D34796 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34725: Add sample PGO integration test to cover profile annotation and inlining.

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D34725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34725: Add sample PGO integration test to cover profile annotation and inlining.

davidxl added inline comments. Comment at: test/CodeGen/pgo-sample.c:4 +// Ensure Pass SampleProfileLoader is invoked. +// RUN: %clang_cc1 -O2 -fprofile-sample-use=%S/Inputs/pgo-sample.prof %s -mllvm -debug-pass=Structure -mllvm -inline-threshold=0 -emit-llvm -o - 2>&1 |

[PATCH] D34663: Update test for enabling ICP for AutoFDO.

davidxl added inline comments. Comment at: test/CodeGen/pgo-sample-thinlto-summary.c:39 +// O2: if.true.direct_targ // ThinLTO-NOT: if.true.direct_targ void icp(void (*p)()) { Is the thinLTO behavior here expected? https://reviews.llvm.org/D34663

[PATCH] D34663: Update test for enabling ICP for AutoFDO.

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D34663 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34796: upporting -f(no)-reorder-functions flag, clang side change

davidxl added a comment. The patch itself is fine. The meta question is whether we expect this option to be generally useful? https://reviews.llvm.org/D34796 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34133: Cold attribute on function decls are lost. Fix the problem

davidxl created this revision. Herald added a subscriber: sanjoy. LLVM static branch prediction depends on cold attribute to annotate branch probability. This is currently not possible for cold function decls as the information is dropped by FE. This patch attaches the attributes to the

[PATCH] D34133: Cold attribute on function decls are lost. Fix the problem

This revision was automatically updated to reflect the committed changes. Closed by commit rL305325: Preserve cold attribute for function decls (authored by davidxl). Changed prior to commit: https://reviews.llvm.org/D34133?vs=102358=102409#toc Repository: rL LLVM

[PATCH] D34082: [Frontend 'Show hotness' can be used with a sampling profile

davidxl added inline comments. Comment at: test/Frontend/optimization-remark-with-hotness.c:32 +// RUN: -Rpass-analysis=inline -fdiagnostics-show-hotness 2>&1 | FileCheck \ +// RUN: -check-prefix=PGO_ENABLED %s +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9

[PATCH] D34133: Cold attribute on function decls are lost. Fix the problem

davidxl updated this revision to Diff 102358. davidxl added a comment. Herald added a subscriber: javed.absar. Add a test case. https://reviews.llvm.org/D34133 Files: lib/CodeGen/CGCall.cpp test/CodeGen/attributes.c Index: test/CodeGen/attributes.c

[PATCH] D34082: [Frontend] 'Show hotness' can be used with a sampling profile

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D34082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D38124: Hide some symbols to avoid a crash on shutdown when using code coverage

davidxl added a comment. Can you add a test case with shared libraries? https://reviews.llvm.org/D38124 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37091: Expose -mllvm -accurate-sample-profile to clang.

davidxl added a comment. Looks fine to me, but please wait for Richard's comment. https://reviews.llvm.org/D37091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37091: Expose -mllvm -accurate-sample-profile to clang.

davidxl accepted this revision. davidxl added a comment. lgtm https://reviews.llvm.org/D37091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37091: Expose -mllvm -accurate-sample-profile to clang.

davidxl added a comment. Documentation needs to be added to clang/docs/ClangCommandLineReference.rst . There probably also needs some kind of testing for the option processing: see clang_f_opts.c https://reviews.llvm.org/D37091 ___ cfe-commits

[PATCH] D37091: Expose -mllvm -accurate-sample-profile to clang.

davidxl added inline comments. Comment at: docs/ClangCommandLineReference.rst:176 + +If sample profile is accurate, we will mark all un-sampled callsite as cold. Otherwise, treat un-sampled callsites as if we have no profile + If the sample profile is accurate,

[PATCH] D40568: design document for a hardware-assisted memory safety (HWAMS) tool, similar to AddressSanitizer

davidxl added inline comments. Comment at: docs/TaggedAddressSanitizerDesign.rst:22 +*quarantine* to find use-after-free. +The shadow, the redzones, and the quarantine are the +major sources of AddressSanitizer's memory overhead. What is the overhead of redzones

[PATCH] D45454: Add llvm_gcov_flush to be called outside a shared library

davidxl added inline comments. Comment at: lib/profile/GCDAProfiling.c:546 +// find and call. In that case, it dumps profile data of a .so file. +// If it is called directly inside a .so file, the unified copy of +// llvm_gcov_flush might dump data of other .so file or the main

[PATCH] D45454: Add llvm_gcov_flush to be called outside a shared library

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45454: Make __gcov_flush visible outside a shared library

davidxl added a comment. GCC's behavior is not documented and it also has changed over the years. Unless there is a bug, changing LLVM's gcov_flush visibility can potentially break clients that depend on this behavior. https://reviews.llvm.org/D45454

[PATCH] D45454: Add llvm_gcov_flush to be called outside a shared library

davidxl added a comment. With the current gcov_flush implementation in LLVM, making gcov_flush's visibility to be default will simply lead to wrong behavior. GCC libgcov's implementation is more elaborate -- it allows gcov_flush to dump gcda data for all dynamic objects while making sure

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

davidxl added a comment. thanks for working on this. Can you split the patch into two? One for sample PGO and one for instrumentation. In particular, I don't see much need to do this for instrumentation based PGO, but we can defer discussion on that once the patch is split.

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

davidxl added a comment. Re "Why not": The common use model of instrumentation based PGO and SamplePGO are quite different. The former usually uses 'fresh' profile that matches the source. If there are massive code refactoring or ABI changes, the user can simply regenerate the profile. For

[PATCH] D34796: upporting -f(no)-reorder-functions flag, clang side change

davidxl added inline comments. Comment at: docs/ClangCommandLineReference.rst:1750 + +Add section prefixes for hot/cold functions + prefix or suffix? Or just leave the details out (also consider the interaction with -ffunction-sections)? Consider documenting:

[PATCH] D45454: Make __gcov_flush visible outside a shared library

davidxl added a comment. With this change, gcov_flush will resolve to the copy in the main executable for each shared library -- this is not the desired the behavior. https://reviews.llvm.org/D45454 ___ cfe-commits mailing list

[PATCH] D45454: Make __gcov_flush visible outside a shared library

davidxl added a comment. The behavior will change if the shared libraries are not 'dlopen'ed but loaded at program startup time. https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

davidxl added inline comments. Comment at: docs/UsersManual.rst:1792 + The profile generated by ``-fcs-profile-generate`` and ``-fprofile-generate`` + can be merged by llvm-profdata. + Please add a use example to show the typical flow with this option (both

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54176/new/ https://reviews.llvm.org/D54176 ___ cfe-commits mailing list

[PATCH] D54176: [PGO] clang part of change for context-sensitive PGO.

davidxl added a comment. Please add changes to option documentation. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54176/new/ https://reviews.llvm.org/D54176 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

davidxl added a comment. Is there a bug reference somewhere? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63626/new/ https://reviews.llvm.org/D63626 ___ cfe-commits mailing list

[PATCH] D63155: [clang][NewPM] Fix broken profile test

davidxl added a comment. Rong, can you take a look at this patch? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63155/new/ https://reviews.llvm.org/D63155 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D69591: Devirtualize a call on alloca without waiting for post inline cleanup and next DevirtSCCRepeatedPass iteration.

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69591/new/ https://reviews.llvm.org/D69591

[PATCH] D69591: Devirtualize a call on alloca without waiting for post inline cleanup and next DevirtSCCRepeatedPass iteration.

davidxl added inline comments. Comment at: llvm/test/Transforms/Inline/devirtualize-4.ll:2 +; RUN: opt < %s -passes='cgscc(devirt<4>(inline)),function(sroa,early-cse)' -S | FileCheck %s +; UN: opt < %s -passes='default' -S | FileCheck %s + typo ? UN:

[PATCH] D73307: Unique Names for Functions with Internal Linkage

davidxl added inline comments. Comment at: clang/docs/UsersManual.rst:1683 + Controls whether Clang emits a unique (best-effort) symbol name for internal + linkage symbols. The unique name is obtained by appending the MD5 hash of the + full module name to the original

[PATCH] D73307: Unique Names for Functions with Internal Linkage

davidxl added inline comments. Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:29 +int mver_call() { + return mver(); +} [a side note]: interesting, since when Clang had this target attribute based multi-versioning and function overloading ?

[PATCH] D79830: Add support of __builtin_expect_with_probability

davidxl added a comment. Is it possible to overload __builtin_expect(..)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list

[PATCH] D73307: Unique Names for Functions with Internal Linkage

davidxl added a comment. As far as I understand, this is not a propeller specific patch, but it fixes a general problem in AFDO as well. Perhaps change the summary to reflect that? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73307/new/ https://reviews.llvm.org/D73307

[PATCH] D73307: Unique Names for Functions with Internal Linkage

davidxl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1135 +llvm::MD5 Md5; +Md5.update(getModule().getSourceFileName()); +llvm::MD5::MD5Result R; rnk wrote: > davidxl wrote: > > Source filenames are not guaranteed to be

[PATCH] D73307: Unique Names for Functions with Internal Linkage

davidxl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1129 + // should get unique names. Use the hash of module name to get a unique + // identifier and this is a best effort. + if (getCodeGenOpts().UniqueInternalFuncNames &&

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

davidxl added a comment. For x86 target, should it be turned on when -fprofile-use= option is specified unless -fno-split-machine-function is specified? Also is it worth to give a warning if the option is specified but PGO is not on? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D87622: [MemProf] Rename HeapProfiler to MemProfiler for consistency

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. LGTM (if the option is documented, the documentation part also needs to be updated). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D86193: [CSSPGO] Pseudo probe instrumentation for basic blocks.

davidxl added a comment. A heads up -- I won't be able to review patch until mid Sept. Hope this is fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86193/new/ https://reviews.llvm.org/D86193 ___

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

davidxl added a comment. I think the user visible option needs to match the functionality. Internal naming just needs some comments to document. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm. The user option should be documented after the runtime is ready. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:71 + +// This flag may need to be

[PATCH] D87737: Add -fprofile-update={atomic,prefer-atomic,single}

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. Looks good. Makes the tsan and instrumentation interaction also cleaner. Comment at: clang/test/CodeGen/code-coverage-tsan.c:1 -/// -fsanitize=thread requires the

[PATCH] D87737: Add -fprofile-update={atomic,prefer-atomic,single}

davidxl added a comment. Perhaps also add clang option manual description. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87737/new/ https://reviews.llvm.org/D87737 ___ cfe-commits mailing list

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm (with the small test enhancement) Comment at: llvm/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext:20 18 12 nit: change 12 to a

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl accepted this revision. davidxl added a comment. lgtm Just realized that we need a test case to show it fixes the original issue (existence with memop --> different hash). Ok as a follow up . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added inline comments. Comment at: llvm/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext:1 # IR level Instrumentation Flag :ir We can convert this test case to cover the new option introduced: basically create another profile entry

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

davidxl added a comment. one nit: since the same instrumentation can be used to profiling global variable accesses (especially those indirect accessed), the option name seems excluding those cases. Shall it be renamed to fmem-prof? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

davidxl added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:512 + +int ModuleHeapProfiler::GetHeapProfVersion(const Module ) const { return 1; } + define a macro for the version number? Repository: rG LLVM Github Monorepo

[PATCH] D82213: [Remarks] Add callsite locations to inline remarks

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:383 + const Function , const InlineCost , + bool

[PATCH] D82213: [Remarks] Add callsite locations to inline remarks

davidxl added a comment. Can you add a test case where there is more than one level of inline contexts for the callsite? Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:391 +Remark << ore::NV("Caller", ); +if (ProfileGuidedInline) + Remark << " by profile guided

[PATCH] D82213: [Remarks] Add callsite locations to inline remarks

davidxl added inline comments. Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:392 +if (ProfileGuidedInline) + Remark << " by profile guided inliner"; +Remark << " with " << IC; Perhaps reword it to " to match profiling context" .. Repository:

[PATCH] D79830: Add support of __builtin_expect_with_probability

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. LLVM side of change LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

davidxl added inline comments. Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:58 +std::pair setBranchWeight(Intrinsic::ID IntrinsicID, + CallInst *CI, int BranchCount) { nit: change name to

[PATCH] D79830: Add support of __builtin_expect_with_probability

davidxl added a comment. Can you add some tests at the LLVM side? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. changes like in llvm/test/Transforms/PGOProfile/PR41279.ll etc can be independently committed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. We may also need to bump both the raw and index format version with this change. For the profile use side, we also need to keep the hashing scheme of the older version (in profile-use side). More details to come. Many tests can also be enhanced to filter out the hash

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. Can you split out the test changes and commit it separately? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 ___ cfe-commits mailing

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. On version bump --- looks like FrontPGO has been bumping version for hashing changes while IR PGO had not, so it is ok to leave the version unchanged (current situation is also confusing as the version of IR and FE PGO are mixed). On the other hand, just in case, we

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89087/new/ https://reviews.llvm.org/D89087

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

davidxl added a comment. Herald added a subscriber: dexonsmith. There should be a related LLVM side of changes. Is it in a different patch? Comment at: clang/lib/CodeGen/CodeGenModule.cpp:587 - if (CodeGenOpts.CFProtectionBranch && -

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

davidxl added a comment. longer term, the profile will be dumped into PGO's raw file, so for now is there a need for a user level option? should an internal option good enough? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89087/new/

[PATCH] D94820: Support for instrumenting only selected files or functions

davidxl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2591 + } + return false; +} phosek wrote: > davidxl wrote: > > If the profile list contains only one line of exclude list, it seems that > > all functions will be rejected as the

[PATCH] D94820: Support for instrumenting only selected files or functions

davidxl added inline comments. Comment at: clang/test/CodeGen/profile-filter.c:17 +// RUN: %clang_cc1 -fprofile-instrument=clang -fprofile-list=%t-exclude-only.list -emit-llvm %s -o - | FileCheck %s --check-prefix=EXCLUDE + +unsigned i; phosek wrote: > davidxl

[PATCH] D94820: Support for instrumenting only selected files or functions

davidxl added inline comments. Comment at: clang/test/CodeGen/profile-filter.c:17 +// RUN: %clang_cc1 -fprofile-instrument=clang -fprofile-list=%t-exclude-only.list -emit-llvm %s -o - | FileCheck %s --check-prefix=EXCLUDE + +unsigned i; Can you add a test case

[PATCH] D94820: Support for instrumenting only selected files or functions

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. my bad -- I missed the test case. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94820/new/ https://reviews.llvm.org/D94820 ___

[PATCH] D94820: Support for instrumenting only selected files or functions

davidxl added inline comments. Comment at: clang/docs/SourceBasedCodeCoverage.rst:77 +the files and functions specified in ``file.list`` will be instrumented. The +option can be specified multiple times to pass multiple files: + perhaps documenting how compiler

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

davidxl added a comment. Adding Wei to help measure performance impact on our internal workloads. Also add Wenlei to help measure impact with FB's workloads. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104099/new/

[PATCH] D104871: [Docs] use -fprofile-generate for IR PGO and -fprofile-instr-generate for code coverage

davidxl added inline comments. Comment at: clang/docs/UsersManual.rst:1881 conversion tool that can convert one to the other. So, a profile generated - via ``-fprofile-instr-generate`` must be used with ``-fprofile-instr-use``. + via ``-fprofile-generate`` must be used

[PATCH] D102818: [PGO] Don't reference functions unless value profiling is enabled

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm (value profiling is off by default for FE PGO anyway) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102818/new/

[PATCH] D98061: [InstrProfiling] Generate runtime hook for ELF platforms

davidxl added a comment. Is the case when there is no counters very rare? And for those cases, how much overhead the runtime hook can incur? I assume it is small compared with actual instrumentation? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D98061: [InstrProfiling] Generate runtime hook for ELF platforms

davidxl added a comment. thanks for the background. This patch looks good at higher level. Vedant can help detailed review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98061/new/ https://reviews.llvm.org/D98061

[PATCH] D96919: [clang] Emit type metadata on available_externally vtables for WPD

davidxl added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1770 - if (!VTable->isDeclarationForLinker()) + if (!VTable->isDeclarationForLinker() || + CGM.getCodeGenOpts().WholeProgramVTables) { Can you add some comment here -- also

[PATCH] D96919: [clang] Emit type metadata on available_externally vtables for WPD

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1781 +// analysis. +if (VTable->isDeclarationForLinker()) + CGM.addCompilerUsedGlobal(VTable);

[PATCH] D96919: [clang] Emit type metadata on available_externally vtables for WPD

davidxl added a comment. So in this case, the virtual call in user code is resolved to a strong definition in the shared library even though user code also has a derived class defined? In other words, the library provides another overrider definition as the final overrider? Repository: rG

[PATCH] D96919: [clang] Emit type metadata on available_externally vtables for WPD

davidxl added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1781 +// analysis. +if (VTable->isDeclarationForLinker()) + CGM.addCompilerUsedGlobal(VTable); Is it better to guard it with with if

[PATCH] D120504: [AST] RAV doesn't traverse explicitly instantiated function bodies by default

davidxl added inline comments. Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:157 + bool shouldVisitTemplateInstantiations() { return true; } + This one line change looks ok to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

davidxl added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:569 + if (!containsProfilingIntrinsics(M)) { +if (!CoverageNamesVar || !NeedsRuntimeHook) { + return MadeChange; gulfem wrote: > MaskRay wrote: > >

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

davidxl added a comment. Should this be fixed in Clang (not generating coverage record)? Adding the logic here is a little confusing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122336/new/ https://reviews.llvm.org/D122336

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

davidxl added a comment. For coverage mode, why using 'incrementProfileCounter'? Should it be set to '1' instead? Also is the 'or' expression needed? The expression can be folded to either zero or 1. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

davidxl added inline comments. Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:606 +} else { + StaticFuncMap[NewName] = "---"; +} define a macro for the marker string. Comment at:

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132600/new/ https://reviews.llvm.org/D132600 ___ cfe-commits mailing list

[PATCH] D132600: [llvm-profdata] Handle internal linkage functions in profile supplementation

davidxl added inline comments. Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:564 + auto buildStaticFuncMap = [, + SampleProfileHasFUnique](const StringRef ) { +std::string Prefixes[] = {".cpp:", "cc:", ".c:", ".hpp:", ".h:"};

[PATCH] D130933: Add docs for function attributes hot/cold

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. LGTM from the content point of view. Please also address aaron's comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130933/new/

[PATCH] D149123: [AArch64][InlineAsm]Add Clang support for flag output constraints

davidxl added inline comments. Comment at: clang/lib/Basic/Targets/AArch64.cpp:1216 +// Returns the length of cc constraint. +static unsigned matchAsmCCConstraint(const char *) { + constexpr unsigned len = 5; Name is not modified in this method, so perhaps

[PATCH] D155290: [PGO] Use Unique Profile Files when New Processes are Forked

davidxl added inline comments. Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:795 - /* When PNS >= OldPNS, the last one wins. */ - if (!FilenamePat || parseFilenamePattern(FilenamePat, CopyFilenamePat)) resetFilenameToDefault(); what is this

[PATCH] D149800: [WIP][PGO] Add ability to mark cold functions as optsize/minsize/optnone

davidxl added a comment. Sorry I missed the question. What I meant is that a lot of the profile guided size optimization (PGSO) done by Hiroshi can be adapted to use the attribute based method proposed in the patch as we don't need two different mechanisms. Repository: rG LLVM Github

[PATCH] D142994: [UsersManual] Add llvm-progen as an alternative tool for AutoFDO profile generation.

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142994/new/ https://reviews.llvm.org/D142994

[PATCH] D149800: [WIP][PGO] Add ability to mark cold functions as optsize/minsize/optnone

davidxl added a comment. This patch uses a cleaner method than the previous effort. There is some differences: 1. yamauchi's patch works for both iFDO and autoFDO, while this patch limits to iFDO 2. yamauchi's patch also handles size optimization (IIRC) at granularity smaller than function

[PATCH] D150295: [MemProf] Update hot/cold information after importing

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150295/new/ https://reviews.llvm.org/D150295