[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 change for? Comment at: compiler-rt/test/profile/Posix/instrprof-fork.c:10 +// RUN: PROFILE2=`tail -n 1 %t.out` +// RUN: ls $PROFILE2 + add some dump and check to make sure profiles are not truncated? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155290/new/ https://reviews.llvm.org/D155290 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149800/new/ https://reviews.llvm.org/D149800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 Longer term, the previous functionality should fold into the new framework. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149800/new/ https://reviews.llvm.org/D149800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 *&Name) {
+ constexpr unsigned len = 5;
Name is not modified in this method, so perhaps dropping '&'?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149123/new/
https://reviews.llvm.org/D149123
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 = [&StaticFuncMap,
+ SampleProfileHasFUnique](const StringRef &Name) {
+std::string Prefixes[] = {".cpp:", "cc:", ".c:", ".hpp:", ".h:"};
no need for & for StringRef
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132600/new/
https://reviews.llvm.org/D132600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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: llvm/tools/llvm-profdata/llvm-profdata.cpp:655
const sampleprof::FunctionSamples &FS = PD.second;
auto It = InstrProfileMap.find(FContext.toString());
+if (It == InstrProfileMap.end()) {
Skip to the next (using continue) when FS is not interesting (cold etc)
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:667
+}
+if (FS.getMaxCountInside() > ColdSampleThreshold &&
It != InstrProfileMap.end() &&
continue when Itr == end or when Itr is not cold
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132600/new/
https://reviews.llvm.org/D132600
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/ https://reviews.llvm.org/D130933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 https://reviews.llvm.org/D126586/new/ https://reviews.llvm.org/D126586 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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:
> > https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
> >
> > That said, nested if is usually written as a single if with `&&`
> Thanks for the tip @MaskRay, and I'm going to fix that in a following patch.
It would be helpful to add a comment in the code reflecting the review
discussions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122336/new/
https://reviews.llvm.org/D122336
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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
https://reviews.llvm.org/D120504/new/
https://reviews.llvm.org/D120504
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 with ``-fprofile-instr-use``. Similarly, sampling profiles generated by external profilers must be This is not correct. As of today, -fprofile-use can be used with both -fprofile-generate and -fprofile-instr-generate. It does this by looking at the flag in the profile data. The better way to document: For profile guided optimizations, we recommend using IRPGO, aka using-fprofile-generate with -fprofile-use Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104871/new/ https://reviews.llvm.org/D104871 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/ https://reviews.llvm.org/D104099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/ https://reviews.llvm.org/D102818 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 https://reviews.llvm.org/D98061/new/ https://reviews.llvm.org/D98061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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); tejohnson wrote: > davidxl wrote: > > Is it better to guard it with with if > > (CGM.getCodeGenOpts().WholeProgramVTables) which seems clearer in > > intention? > No I don't think we want to do this, it would mean every single vtable would > be put on the llvm.compiler.used which is unnecessary. The > available_externally are the ones that aren't sticking around otherwise. Ok. Perhaps add an assertion that wholeProgram is on in this case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96919/new/ https://reviews.llvm.org/D96919 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 (CGM.getCodeGenOpts().WholeProgramVTables) which seems clearer in intention? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96919/new/ https://reviews.llvm.org/D96919 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 help user understand the code. Note that
isDeclarationForLinker is not intuitive by name.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96919/new/
https://reviews.llvm.org/D96919
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96919/new/ https://reviews.llvm.org/D96919 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 wrote: > > Can you add a test case with both include and exclude list (the > > intersection of INCLUDE_SET & !EXCLUDE_SET)? > See the case above this one which has: > ``` > fun:test* > !fun:test1 > ``` > Is that what you have in mind? Right CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94820/new/ https://reviews.llvm.org/D94820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 with both include and exclude list (the intersection of INCLUDE_SET & !EXCLUDE_SET)? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94820/new/ https://reviews.llvm.org/D94820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 function returns 'false' by default > > for all other functions? > That's correct, would you expect a different behavior and if so what should > that behavior be? > > Currently, the profile list is used to build up a list of functions and files > that should be instrumented. Any function or file that's not covered by the > profile list is automatically excluded (not included may be more correct). > Having just excludes without any includes means that nothing will be > instrumented. It is natural and more useful to let user simply specify exclude lists so they don't need to worry about patterns of other functions to be instrumented. This means that when only exclude list is specified, the default should be flipped to be true (instrument everything else). If there only include list, the default is false. If there are both, the include list should be used to specify a super set while the exclude list (taking precedence) used to specify a subset from the super set. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94820/new/ https://reviews.llvm.org/D94820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 generated functions are handled? Comment at: clang/docs/SourceBasedCodeCoverage.rst:86 +.. code-block:: none + + # all functions whose name starts with foo will be instrumented. what is the default section name? Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2569 + // If the profile list is empty, then instrument everything. + if (ProfileList.isEmpty()) +return true; Should the option to turn on instrumentation also be checked? Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2591 + } + return false; +} If the profile list contains only one line of exclude list, it seems that all functions will be rejected as the function returns 'false' by default for all other functions? Comment at: clang/lib/CodeGen/CodeGenModule.h:1280 + bool isProfileInstrumented(llvm::Function *Fn, SourceLocation Loc, + StringRef Section = StringRef()) const; Document the method and params. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94820/new/ https://reviews.llvm.org/D94820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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/ https://reviews.llvm.org/D89087 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 &&
- Target.checkCFProtectionBranchSupported(getDiags())) {
-// Indicate that we want to instrument branch control flow protection.
-getModule().addModuleFlag(llvm::Module::Override, "cf-protection-branch",
+ if (CodeGenOpts.CFProtectionReturn &&
+ Target.checkCFProtectionReturnSupported(getDiags())) {
Is this change relevant?
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:613
+llvm::LLVMContext &Ctx = TheModule.getContext();
+getModule().addModuleFlag(
+llvm::Module::Error, "MemProfProfileFilename",
Is there a need for a module flag? PGO passes an InstrProfOptions typed object
to the instrumentation pass. The option object has the directory info.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89087/new/
https://reviews.llvm.org/D89087
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 (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. Fix typo: atmomic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87737/new/ https://reviews.llvm.org/D87737 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 https://reviews.llvm.org/D87622/new/ https://reviews.llvm.org/D87622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ACTION https://reviews.llvm.org/D87047/new/ https://reviews.llvm.org/D87047 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 replaced with -f[no-]heapprof-reads.
+static cl::opt ClInstrumentReads("heapprof-instrument-reads",
-f[no-]memprof-reads?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85948/new/
https://reviews.llvm.org/D85948
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 &M) const { return 1; }
+
define a macro for the version number?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85948/new/
https://reviews.llvm.org/D85948
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 different value say 6.(otherwise if the option is an nop,
the test will still succeed).
Comment at: llvm/test/Transforms/PGOProfile/multiple_hash_profile.ll:3
; RUN: opt < %s -pgo-instr-use -pgo-test-profile-file=%t.profdata -S |
FileCheck %s
+; RUN: opt < %s -pgo-instr-use -pgo-test-profile-file=%t.profdata
-pgo-instr-old-cfg-hashing=true -S | FileCheck %s
; RUN: opt < %s -passes=pgo-instr-use -pgo-test-profile-file=%t.profdata -S |
FileCheck %s
Use a different prefix like CHECKOLD
Comment at: llvm/test/Transforms/PGOProfile/multiple_hash_profile.ll:33
; CHECK-SAME: !prof ![[BW:[0-9]+]]
; CHECK: ![[BW]] = !{!"branch_weights", i32 12, i32 6}
%retval.0.i = mul nsw i32 %mul.i, %i
CHECKOLD expects a different weight.
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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 for _Z3fooi with the old hash. Change the counter value a little and expect the profile-use match the old entry when the option is used. 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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 need to introduce a temporary internal option for people to be able to keep on using old profile (with older scheme of hashing) for a while. Basically, under the option, the hashing scheme falls back to the old way. other than that, looks fine. 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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 details if they are not part the main test. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:623 // Compute Hash value for the CFG: the lower 32 bits are CRC32 of the index -// value of each BB in the CFG. The higher 32 bits record the number of edges. +// value of each BB in the CFG. The higher 32 bits are the CR32 of the numbers +// of selects, indirect calls, mem ops and edges. MaskRay wrote: > To make sure I understand: we use two CRCs instead of one CRC because we want > to extract low/high 32 bits for debugging purposes? > > Otherwise, we can just use one CRC. JamCRC produces a 32bit value. 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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 &Caller, const InlineCost &IC,
+ bool ProfileGuidedInline, const char *PassName) {
ORE.emit([&]() {
ProfileGuidedInline --> ForProfileContext
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82213/new/
https://reviews.llvm.org/D82213
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82213/new/ https://reviews.llvm.org/D82213 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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", &Caller);
+if (ProfileGuidedInline)
+ Remark << " by profile guided inliner";
is this necessary? User should know if their build has profile or not.
What is more useful is when PGO is on, but some callsite does not have profile
data, then it is worth reporting.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82213/new/
https://reviews.llvm.org/D82213
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/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 getBranchWeight or computeBranchWeight
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:69
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
assert TrueProb is in [0,1]
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:96
+ uint32_t LikelyBranchWeightVal = WeightNums.first;
+ uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
simpler to use tuple and tie here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79830/new/
https://reviews.llvm.org/D79830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ? Sriraman added this support in GCC
8 years ago. Did not realize it is in Clang too :)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73307/new/
https://reviews.llvm.org/D73307
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 symbol. This option is particularly useful Is it necessary to document 'MD5 hash' as it is an implementation detail? Perhaps just 'the name hash' Comment at: clang/docs/UsersManual.rst:1689 + It should be noted that this option cannot guarantee uniqueness and the + following is an example where it is not unique: + if the two modules contain symbols with the same private linkage name. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1060 + const Decl *D = GD.getDecl(); + if (CGM.getCodeGenOpts().UniqueInternalLinkageNames && + !CGM.getModuleNameHash().empty() && is the first check redundant now? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73307/new/ https://reviews.llvm.org/D73307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 unique, or it does contain the > > path as well? > It appears to contain the path as the compiler receives it on the command > line. However, it is possible to design a build where this string is not > unique: > ``` > cd foo > clang -c name_conflict.c > cd ../bar > clang -c name_conflict.c > clang foo/name_conflict.o bar/name_conflict.o > ``` > > I don't think we should try to handle this case, but we should document the > limitation somewhere. Some build systems do operate changing the working > directory as they go. > > Can you add some to the clang/docs/UsersManual.rst file? I'd expect it to > show up here: > https://clang.llvm.org/docs/UsersManual.html#command-line-options > > You can elaborate that the unique names are calculated using the source path > passed to the compiler, and in a build system where that is not unique, the > function names may not be unique. > > --- > > I have also used this construct in the past for building single-file ABI > compatibility test cases: > > ``` > // foo.cpp > #ifdef PART1 > // ... > #elif defined(PART2) > // ... > #endif > > $ cc -c foo.cpp -DPART1 > $ cc -c foo.cpp -DPART2 > ``` yes, the first example was the scenario I meant. I agree name conflict due that case should be really rare. If yes, we can always use content based uniqueid -- by appending llvm::getUniqueModuleId -- but that is probably overkill. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73307/new/ https://reviews.llvm.org/D73307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 && lebedev.ri wrote: > maybe > `identifier as this is a best-effort solution` > Address review comment here. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1135 +llvm::MD5 Md5; +Md5.update(getModule().getSourceFileName()); +llvm::MD5::MD5Result R; Source filenames are not guaranteed to be unique, or it does contain the path as well? Comment at: clang/test/CodeGen/unique-internal-funcnames.c:3 + +// RUN: %clang -target x86_64 -S -o - %s | FileCheck %s --check-prefix=PLAIN +// RUN: %clang -target x86_64 -S -funique-internal-funcnames -o - %s | FileCheck %s --check-prefix=UNIQUE MaskRay wrote: > You can hardly find any .c -> .s test in clang/test. We mostly do .c -> .ll > testing. `.ll` -> `.s` are in llvm/test/CodeGen. Is this convention documented somewhere? Any concerns of producing .s? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73307/new/ https://reviews.llvm.org/D73307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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: Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69591/new/ https://reviews.llvm.org/D69591 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 gen and use phases -- including llvm-profdata step). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54176/new/ https://reviews.llvm.org/D54176 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 the latter, the profile data is usually collected from production binaries, so the source is almost always newer than the profile. https://reviews.llvm.org/D51240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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. https://reviews.llvm.org/D51240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 module. Just document this interface has non-hidden visibility and will resolve to a unified copy. The right underlying behavior is for it to dump profiles from all dynamic modules, but it is not there until Marco's patch is in. In other words, do not need to document the current behavior for now. https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 gcov_exit only dumps the gcda files from the same dynamic module (otherwise, there will be wrong profile merging). This is done via two levels of linked list. The patch https://reviews.llvm.org/D48538 is in the right direction, but not complete yet. https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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: 1) what is the default behavior? 2) how does it affect function ordering? 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] 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 compared with shadow memory? Comment at: docs/TaggedAddressSanitizerDesign.rst:49 +--- +All memory accesses are prefixed with a call to a run-time function +that loads the memory tag, compares it with the a real runtime call or it will be lowered into inline sequence? Repository: rC Clang https://reviews.llvm.org/D40568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 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. 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 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, callsites without profile samples are marked as cold. Otherwise, ..., This option can be used to enable more aggressive size optimization based on profiles. Comment at: include/clang/Driver/Options.td:593 + HelpText<"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">; un-sampled callsites --> callsites without profile samples 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 mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 |
FileCheck %s
// CHECK: Remove unused exception handling info
using a a negative threshold to make sure regular inline heuristic does not
kick in?
Also how about new PM?
Comment at: test/CodeGen/pgo-sample.c:28
+// of foo:bar.
+// CHECK-NOT: call void @callee
+void foo(int x) {
SHould this be 'CHECK-NOT: call' as bar is also inlined?
https://reviews.llvm.org/D34725
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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&id=102409#toc
Repository:
rL LLVM
https://reviews.llvm.org/D34133
Files:
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/test/CodeGen/attributes.c
Index: cfe/trunk/lib/CodeGen/CGCall.cpp
===
--- cfe/trunk/lib/CodeGen/CGCall.cpp
+++ cfe/trunk/lib/CodeGen/CGCall.cpp
@@ -1795,6 +1795,8 @@
FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
if (TargetDecl->hasAttr())
FuncAttrs.addAttribute(llvm::Attribute::NoReturn);
+if (TargetDecl->hasAttr())
+ FuncAttrs.addAttribute(llvm::Attribute::Cold);
if (TargetDecl->hasAttr())
FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
if (TargetDecl->hasAttr())
Index: cfe/trunk/test/CodeGen/attributes.c
===
--- cfe/trunk/test/CodeGen/attributes.c
+++ cfe/trunk/test/CodeGen/attributes.c
@@ -56,6 +56,13 @@
void t7() __attribute__((noreturn, nothrow));
void t7() { while (1) {} }
+// CHECK: define void @t72() [[COLDDEF:#[0-9]+]] {
+void t71(void) __attribute__((cold));
+void t72() __attribute__((cold));
+void t72() { t71(); }
+// CHECK: call void @t71() [[COLDSITE:#[0-9]+]]
+// CHECK: declare void @t71() [[COLDDECL:#[0-9]+]]
+
// CHECK: define void @t10() [[NUW]] section "SECT" {
void t10(void) __attribute__((section("SECT")));
void t10(void) {}
@@ -92,3 +99,6 @@
// CHECK: attributes [[NUW]] = { noinline nounwind{{.*}} }
// CHECK: attributes [[NR]] = { noinline noreturn nounwind{{.*}} }
+// CHECK: attributes [[COLDDEF]] = { cold {{.*}}}
+// CHECK: attributes [[COLDDECL]] = { cold {{.*}}}
+// CHECK: attributes [[COLDSITE]] = { cold {{.*}}}
Index: cfe/trunk/lib/CodeGen/CGCall.cpp
===
--- cfe/trunk/lib/CodeGen/CGCall.cpp
+++ cfe/trunk/lib/CodeGen/CGCall.cpp
@@ -1795,6 +1795,8 @@
FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
if (TargetDecl->hasAttr())
FuncAttrs.addAttribute(llvm::Attribute::NoReturn);
+if (TargetDecl->hasAttr())
+ FuncAttrs.addAttribute(llvm::Attribute::Cold);
if (TargetDecl->hasAttr())
FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
if (TargetDecl->hasAttr())
Index: cfe/trunk/test/CodeGen/attributes.c
===
--- cfe/trunk/test/CodeGen/attributes.c
+++ cfe/trunk/test/CodeGen/attributes.c
@@ -56,6 +56,13 @@
void t7() __attribute__((noreturn, nothrow));
void t7() { while (1) {} }
+// CHECK: define void @t72() [[COLDDEF:#[0-9]+]] {
+void t71(void) __attribute__((cold));
+void t72() __attribute__((cold));
+void t72() { t71(); }
+// CHECK: call void @t71() [[COLDSITE:#[0-9]+]]
+// CHECK: declare void @t71() [[COLDDECL:#[0-9]+]]
+
// CHECK: define void @t10() [[NUW]] section "SECT" {
void t10(void) __attribute__((section("SECT")));
void t10(void) {}
@@ -92,3 +99,6 @@
// CHECK: attributes [[NUW]] = { noinline nounwind{{.*}} }
// CHECK: attributes [[NR]] = { noinline noreturn nounwind{{.*}} }
+// CHECK: attributes [[COLDDEF]] = { cold {{.*}}}
+// CHECK: attributes [[COLDDECL]] = { cold {{.*}}}
+// CHECK: attributes [[COLDSITE]] = { cold {{.*}}}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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
===
--- test/CodeGen/attributes.c
+++ test/CodeGen/attributes.c
@@ -56,6 +56,13 @@
void t7() __attribute__((noreturn, nothrow));
void t7() { while (1) {} }
+// CHECK: define void @t72() [[COLDDEF:#[0-9]+]] {
+void t71(void) __attribute__((cold));
+void t72() __attribute__((cold));
+void t72() { t71(); }
+// CHECK: call void @t71() [[COLDSITE:#[0-9]+]]
+// CHECK: declare void @t71() [[COLDDECL:#[0-9]+]]
+
// CHECK: define void @t10() [[NUW]] section "SECT" {
void t10(void) __attribute__((section("SECT")));
void t10(void) {}
@@ -92,3 +99,6 @@
// CHECK: attributes [[NUW]] = { noinline nounwind{{.*}} }
// CHECK: attributes [[NR]] = { noinline noreturn nounwind{{.*}} }
+// CHECK: attributes [[COLDDEF]] = { cold {{.*}}}
+// CHECK: attributes [[COLDDECL]] = { cold {{.*}}}
+// CHECK: attributes [[COLDSITE]] = { cold {{.*}}}
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -1795,6 +1795,8 @@
FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
if (TargetDecl->hasAttr())
FuncAttrs.addAttribute(llvm::Attribute::NoReturn);
+if (TargetDecl->hasAttr())
+ FuncAttrs.addAttribute(llvm::Attribute::Cold);
if (TargetDecl->hasAttr())
FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
if (TargetDecl->hasAttr())
Index: test/CodeGen/attributes.c
===
--- test/CodeGen/attributes.c
+++ test/CodeGen/attributes.c
@@ -56,6 +56,13 @@
void t7() __attribute__((noreturn, nothrow));
void t7() { while (1) {} }
+// CHECK: define void @t72() [[COLDDEF:#[0-9]+]] {
+void t71(void) __attribute__((cold));
+void t72() __attribute__((cold));
+void t72() { t71(); }
+// CHECK: call void @t71() [[COLDSITE:#[0-9]+]]
+// CHECK: declare void @t71() [[COLDDECL:#[0-9]+]]
+
// CHECK: define void @t10() [[NUW]] section "SECT" {
void t10(void) __attribute__((section("SECT")));
void t10(void) {}
@@ -92,3 +99,6 @@
// CHECK: attributes [[NUW]] = { noinline nounwind{{.*}} }
// CHECK: attributes [[NR]] = { noinline noreturn nounwind{{.*}} }
+// CHECK: attributes [[COLDDEF]] = { cold {{.*}}}
+// CHECK: attributes [[COLDDECL]] = { cold {{.*}}}
+// CHECK: attributes [[COLDSITE]] = { cold {{.*}}}
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -1795,6 +1795,8 @@
FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
if (TargetDecl->hasAttr())
FuncAttrs.addAttribute(llvm::Attribute::NoReturn);
+if (TargetDecl->hasAttr())
+ FuncAttrs.addAttribute(llvm::Attribute::Cold);
if (TargetDecl->hasAttr())
FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
if (TargetDecl->hasAttr())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 callsite as noReturn. Another way is to pass the attribute to the function decl. Also need suggestions on how to add a clang test case. https://reviews.llvm.org/D34133 Files: lib/CodeGen/CGCall.cpp Index: lib/CodeGen/CGCall.cpp === --- lib/CodeGen/CGCall.cpp +++ lib/CodeGen/CGCall.cpp @@ -1795,6 +1795,8 @@ FuncAttrs.addAttribute(llvm::Attribute::NoUnwind); if (TargetDecl->hasAttr()) FuncAttrs.addAttribute(llvm::Attribute::NoReturn); +if (TargetDecl->hasAttr()) + FuncAttrs.addAttribute(llvm::Attribute::Cold); if (TargetDecl->hasAttr()) FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate); if (TargetDecl->hasAttr()) Index: lib/CodeGen/CGCall.cpp === --- lib/CodeGen/CGCall.cpp +++ lib/CodeGen/CGCall.cpp @@ -1795,6 +1795,8 @@ FuncAttrs.addAttribute(llvm::Attribute::NoUnwind); if (TargetDecl->hasAttr()) FuncAttrs.addAttribute(llvm::Attribute::NoReturn); +if (TargetDecl->hasAttr()) + FuncAttrs.addAttribute(llvm::Attribute::Cold); if (TargetDecl->hasAttr()) FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate); if (TargetDecl->hasAttr()) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[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 -main-file-name \ anemet wrote: > modocache wrote: > > Ideally, I'd like for this test to be identical to the ones that use > > `-verify` above, save for using `-fprofile-sample-use`. However I couldn't > > figure out how to write `optimization-remark-with-hotness-sample.proftext` > > such that it would produce hotness info for each of the functions. I'm able > > to confirm, using real sampling from another program of mine, that remarks > > using AutoFDO data do indeed show hotness, but this test does not verify > > this in its current state. > > > > Any advice here? I wrote `optimization-remark-with-hotness-sample.proftext` > > based on the format specified in > > https://clang.llvm.org/docs/UsersManual.html#sample-profile-text-format, > > but maybe it's missing something that would allow hotness to be displayed? > I don't have any immediate ideas since I am not familiar with sample-based > profiling unfortunately. I will study this later unless you beat me to it or > David has some ideas. The hotness is based on profile summary, so you need to adjust the bar's entry count and inline instance of foo's count to be very large value to let compiler decide it is hot. https://reviews.llvm.org/D34082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
