Page MenuHomePhabricator

davidxl (David Li)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 13 2015, 9:48 AM (288 w, 3 d)

Recent Activity

Yesterday

davidxl accepted D86988: [Inliner] Run always-inliner in inliner-wrapper.

lgtm

Thu, Oct 22, 3:32 PM · Restricted Project

Wed, Oct 21

davidxl accepted D88609: Use uint64_t for branch weights instead of uint32_t.

lgtm

Wed, Oct 21, 12:45 PM · Restricted Project

Tue, Oct 13

davidxl accepted D89088: [MBP] Add whole chain to BlockFilterSet instead of individual BB.

lgtm

Tue, Oct 13, 5:21 PM · Restricted Project
davidxl added a comment to D89088: [MBP] Add whole chain to BlockFilterSet instead of individual BB.

ok, makes sense. Basically since the inner loop is laid out first, the outer loop can not individually split out the blocks already laid out. What is the manifest of the problem? wrong profile data or compiler crash or something else?

Tue, Oct 13, 3:31 PM · Restricted Project
davidxl accepted D88838: [PGO] Remove the old memop value profiling buckets..

lgtm

Tue, Oct 13, 1:06 PM · Restricted Project, Restricted Project
davidxl added inline comments to D89088: [MBP] Add whole chain to BlockFilterSet instead of individual BB.
Tue, Oct 13, 11:57 AM · Restricted Project

Fri, Oct 9

davidxl added a comment to D89086: [MemProf] Allow the binary to specify the profile output filename.

Profile runtime has 4 ways of setting output: 1) default 2) compiler command line arg 3) environment variable at runtime; and 4) user invocation of runtime API at runtime. The order of the precedence is 4> 3 > 2 > 1. Is the behavior here consistent with that?

Fri, Oct 9, 9:13 AM · Restricted Project
davidxl accepted D89093: [Inliner][NPM] Fix various tests under NPM.

lgtm

Fri, Oct 9, 9:05 AM · Restricted Project
davidxl added a comment to D89093: [Inliner][NPM] Fix various tests under NPM.

Are the new PM inliner issues being looked into too?

Fri, Oct 9, 9:05 AM · Restricted Project

Fri, Oct 2

davidxl added a comment to D86988: [Inliner] Run always-inliner in inliner-wrapper.

Wait a few days in case others have comments.

Fri, Oct 2, 1:12 PM · Restricted Project
davidxl added a comment to D88609: Use uint64_t for branch weights instead of uint32_t.

Then why modifying existing tests at all? How about add an explicit test using i64 type?

Fri, Oct 2, 11:52 AM · Restricted Project
davidxl added a comment to D88609: Use uint64_t for branch weights instead of uint32_t.

Perhaps add a new test to demonstrate it (that i32 profile weight still works)?

Fri, Oct 2, 11:03 AM · Restricted Project
davidxl accepted D88324: [AlwaysInliner] Update BFI when inlining.

lgtm

Fri, Oct 2, 9:51 AM · Restricted Project

Thu, Oct 1

davidxl added inline comments to D88324: [AlwaysInliner] Update BFI when inlining.
Thu, Oct 1, 1:44 PM · Restricted Project

Wed, Sep 30

davidxl added a comment to D88609: Use uint64_t for branch weights instead of uint32_t.

I like the direction of the patch. There is one concern -- it makes the IR not backward compatible which may affect some users. +vsk.

Wed, Sep 30, 1:13 PM · Restricted Project
davidxl added a reviewer for D88609: Use uint64_t for branch weights instead of uint32_t: vsk.
Wed, Sep 30, 1:12 PM · Restricted Project

Fri, Sep 25

davidxl added a comment to D86988: [Inliner] Run always-inliner in inliner-wrapper.

The reason is that the always inliner is still not in-par with regular inliner -- if you look at the IFI setup -- missing BFI information.

Fri, Sep 25, 8:37 AM · Restricted Project
davidxl added a comment to D87737: Add -fprofile-update={atomic,prefer-atomic,single}.

Perhaps also add clang option manual description.

Fri, Sep 25, 8:35 AM · Restricted Project

Thu, Sep 24

davidxl accepted D87737: Add -fprofile-update={atomic,prefer-atomic,single}.

Looks good. Makes the tsan and instrumentation interaction also cleaner.

Thu, Sep 24, 11:28 PM · Restricted Project
davidxl added inline comments to D86988: [Inliner] Run always-inliner in inliner-wrapper.
Thu, Sep 24, 11:19 PM · Restricted Project

Sep 22 2020

davidxl accepted D87802: [MBFIWrapper] Add a new function getBlockProfileCount.

lgtm

Sep 22 2020, 12:36 PM · Restricted Project

Sep 21 2020

davidxl accepted D88067: [AlwaysInliner] Emit optimization remarks.

lgtm

Sep 21 2020, 10:03 PM · Restricted Project
davidxl added inline comments to D88067: [AlwaysInliner] Emit optimization remarks.
Sep 21 2020, 9:41 PM · Restricted Project
davidxl added inline comments to D87802: [MBFIWrapper] Add a new function getBlockProfileCount.
Sep 21 2020, 7:12 PM · Restricted Project
davidxl added inline comments to D87802: [MBFIWrapper] Add a new function getBlockProfileCount.
Sep 21 2020, 2:56 PM · Restricted Project
davidxl added a comment to D86988: [Inliner] Run always-inliner in inliner-wrapper.

I am ok adding an internal option to control this with it being default for now.

Sep 21 2020, 11:14 AM · Restricted Project

Sep 17 2020

davidxl added a comment to D86988: [Inliner] Run always-inliner in inliner-wrapper.

One major problem with this patch is that the AlwaysInliner is dummy, it is not aware of profile information thus won't be able to perform proper profile update after the inlining of always inline callees. This can be an issue for Frontend PGO or when thinLTO/LTO is on (where there is cross module calls to always inline functions -- though it is not common).

Sep 17 2020, 10:40 PM · Restricted Project
davidxl added a comment to D87802: [MBFIWrapper] Add a new function getBlockProfileCount.

Is it possible to add a test case?

Sep 17 2020, 1:38 PM · Restricted Project

Sep 15 2020

davidxl added a comment to D86988: [Inliner] Run always-inliner in inliner-wrapper.

The AlwaysInliner does not operate on DAG, but depend on function order in the module, so it is possible to regress with the change. I don't have strong opinion on this, but you want want to hear others opinion.

Sep 15 2020, 10:13 PM · Restricted Project

Sep 14 2020

davidxl added a comment to D86988: [Inliner] Run always-inliner in inliner-wrapper.

What is the compile time implication?

Sep 14 2020, 11:09 PM · Restricted Project
davidxl accepted D85240: Disable hoisting MI to hotter basic blocks when using pgo.

lgtm

Sep 14 2020, 10:41 PM · Restricted Project
davidxl accepted D87622: [MemProf] Rename HeapProfiler to MemProfiler for consistency.

LGTM (if the option is documented, the documentation part also needs to be updated).

Sep 14 2020, 1:08 PM · Restricted Project, Restricted Project
davidxl added a comment to D86890: [compiler-rt] Remove copy of InstrProfData.inc.

Adding a test to make sure they are in sync will be useful.

Sep 14 2020, 11:25 AM · Restricted Project, Restricted Project

Sep 13 2020

davidxl added a comment to D86890: [compiler-rt] Remove copy of InstrProfData.inc.

Change in this direction is welcome. Added vsk and beanz to comment on the implication on runtime builds.

Sep 13 2020, 12:49 PM · Restricted Project, Restricted Project
davidxl updated subscribers of D86890: [compiler-rt] Remove copy of InstrProfData.inc.
Sep 13 2020, 12:46 PM · Restricted Project, Restricted Project

Sep 11 2020

davidxl added a comment to D85240: Disable hoisting MI to hotter basic blocks when using pgo.

Can you share the performance data ?

Sep 11 2020, 9:09 AM · Restricted Project

Sep 10 2020

davidxl accepted D87435: [PGO] Skip if an IndirectBrInst critical edge cannot be split.

lgtm

Sep 10 2020, 10:01 AM · Restricted Project

Sep 8 2020

davidxl added a comment to D87337: [PGO] De-Optimizing cold functions based on PGO info (PATCH 1/2).

This is in theory similar to profile guided size optimization -- the difference is that optnone may not result in size reduction.

Sep 8 2020, 5:11 PM · Restricted Project
davidxl updated subscribers of D87337: [PGO] De-Optimizing cold functions based on PGO info (PATCH 1/2).
Sep 8 2020, 5:10 PM · Restricted Project

Sep 2 2020

davidxl added a comment to D87047: [clang] Add command line options for the Machine Function Splitter..

For x86 target, should it be turned on when -fprofile-use= option is specified unless -fno-split-machine-function is specified?

Sep 2 2020, 12:22 PM · Restricted Project

Sep 1 2020

davidxl updated subscribers of rG7ed8124d46f9: [HeapProf] Clang and LLVM support for heap profiling instrumentation.

I accidentally threw in a third choice :). Yes -fmemory-profile is fine
which is consistent with -fsanitize=

Sep 1 2020, 7:11 PM
davidxl added a comment to rG7ed8124d46f9: [HeapProf] Clang and LLVM support for heap profiling instrumentation.

I am fine with -fmemory-profiler.

Sep 1 2020, 6:55 PM

Aug 19 2020

davidxl added a comment to D86193: [CSSPGO] Pseudo probe instrumentation for basic blocks..

A heads up -- I won't be able to review patch until mid Sept. Hope this is fine.

Aug 19 2020, 2:00 PM · Restricted Project, Restricted Project
davidxl added a comment to D85989: [X86] Add feature for Fast Short REP MOV (FSRM) for Icelake or newer..

looks good to me. Wait to see if Craig has any more comments.

Aug 19 2020, 9:35 AM · Restricted Project

Aug 18 2020

davidxl accepted D85784: [PGO][PGSO][LV] Fix loop not vectorized issue under profile guided size opts..

lgtm

Aug 18 2020, 10:20 AM · Restricted Project

Aug 17 2020

davidxl updated subscribers of D85781: [BPI] Improve static heuristics for integer comparisons.

I suggest the following steps to commit this patch:

Aug 17 2020, 11:36 AM · Restricted Project

Aug 14 2020

davidxl accepted D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation.

lgtm. The user option should be documented after the runtime is ready.

Aug 14 2020, 5:25 PM · Restricted Project, Restricted Project
davidxl added a comment to D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation.

I think the user visible option needs to match the functionality. Internal naming just needs some comments to document.

Aug 14 2020, 12:38 PM · Restricted Project, Restricted Project
davidxl added a comment to D85784: [PGO][PGSO][LV] Fix loop not vectorized issue under profile guided size opts..

I mean the check llvm::shouldOptimizeForSize(L->getHeader(), PSI, BFI,

PGSOQueryType::IRPass) on loop header.
Aug 14 2020, 10:56 AM · Restricted Project

Aug 13 2020

davidxl added inline comments to D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation.
Aug 13 2020, 10:25 PM · Restricted Project, Restricted Project
davidxl added a comment to D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation.

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?

Aug 13 2020, 10:12 PM · Restricted Project, Restricted Project

Aug 12 2020

davidxl added inline comments to D85781: [BPI] Improve static heuristics for integer comparisons.
Aug 12 2020, 10:22 PM · Restricted Project

Aug 11 2020

davidxl added a comment to D85784: [PGO][PGSO][LV] Fix loop not vectorized issue under profile guided size opts..

If shouldOptimizeForSize is not available for loop analysis, should the check of be removed unconditionally instead of gated upon whether there is a hint?

Aug 11 2020, 2:40 PM · Restricted Project

Aug 10 2020

davidxl added a comment to D79485: [BPI] Improve static heuristics for "cold" paths..

A lot of test changes can probably be extracted out as NFC (to make the string test more robust) -- this will reduce the size of the patch.

Aug 10 2020, 9:50 PM · Restricted Project
davidxl added a comment to D84771: [llvm-profdata] Add intersect/exclude subcommands to aid differential code coverage analysis..

please also document the new commands in docs/CommandGuide/llvm-profdata.rst

Aug 10 2020, 10:56 AM · Restricted Project

Aug 8 2020

davidxl accepted D85597: [PGO] Delete dead comdat renaming code related to GlobalAlias. NFC.

lgtm

Aug 8 2020, 11:25 PM · Restricted Project

Aug 5 2020

davidxl added inline comments to D85368: [llvm][CodeGen] Machine Function Splitter.
Aug 5 2020, 5:23 PM · Restricted Project
davidxl added a comment to D85368: [llvm][CodeGen] Machine Function Splitter.

Regarding reroller -- compiler with PGO will adjust the agressiveness of the unroller based on instruction workset size estimation. Doing this in later pass or in Propeller can help catch cases that are mis-handled.

Aug 5 2020, 4:59 PM · Restricted Project

Aug 4 2020

davidxl added a comment to D85250: [llvm] Expose type and element count-related APIs on TensorSpec.

lgtm (as Eugene pointed out -- use std::accumulate instead ).

Aug 4 2020, 4:58 PM · Restricted Project
davidxl added a comment to D85250: [llvm] Expose type and element count-related APIs on TensorSpec.

What is the use case of the new APIs?

Aug 4 2020, 3:09 PM · Restricted Project
davidxl accepted D84838: [BPI][NFC] Unify handling of normal and SCC based loops.

lgtm

Aug 4 2020, 10:32 AM · Restricted Project

Aug 3 2020

davidxl accepted D84723: [PGO] Move __profc_ and __profvp_ from their own comdat groups to __profd_'s comdat group.

LGTM ( how much size savings can we expect?)

Aug 3 2020, 4:45 PM · Restricted Project
davidxl added a comment to D84723: [PGO] Move __profc_ and __profvp_ from their own comdat groups to __profd_'s comdat group.

It is ok to push this patch, but please update the summary of this patch from https://reviews.llvm.org/D84766 plus the additional improvement bits. Also document the background history a little.

Aug 3 2020, 10:43 AM · Restricted Project
davidxl accepted D85116: [compiler-rt][profile] Fix various InstrProf tests on Solaris.

Looks good to me.

Aug 3 2020, 9:58 AM · Restricted Project

Jul 31 2020

davidxl added inline comments to D84838: [BPI][NFC] Unify handling of normal and SCC based loops.
Jul 31 2020, 9:59 AM · Restricted Project

Jul 30 2020

davidxl accepted D84994: [SampleFDO] Fix a crash when the sample profile uses md5 and -sample-profile-merge-inlinee is enabled.

lgtm

Jul 30 2020, 7:43 PM · Restricted Project
davidxl accepted D84953: [PGO][test] Add test to check memops changes function hash.

lgtm

Jul 30 2020, 11:38 AM · Restricted Project
davidxl added a comment to D84953: [PGO][test] Add test to check memops changes function hash.

I think the test should use profile-gen option to compare generated hash of otherwise identical functions (except for memop).

Jul 30 2020, 10:48 AM · Restricted Project

Jul 29 2020

davidxl added a comment to D84782: [PGO] Include the mem ops into the function hash..

lgtm

Jul 29 2020, 7:10 PM · Restricted Project, Restricted Project, Restricted Project
davidxl updated subscribers of D84782: [PGO] Include the mem ops into the function hash..

right. It occurred to me during review, but did not think of the hard coded
values in proftext depends on LE.

Jul 29 2020, 3:10 PM · Restricted Project, Restricted Project, Restricted Project
davidxl accepted D84782: [PGO] Include the mem ops into the function hash..

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 .

Jul 29 2020, 1:33 PM · Restricted Project, Restricted Project, Restricted Project
davidxl accepted D84782: [PGO] Include the mem ops into the function hash..

lgtm (with the small test enhancement)

Jul 29 2020, 1:06 PM · Restricted Project, Restricted Project, Restricted Project
davidxl added inline comments to D84782: [PGO] Include the mem ops into the function hash..
Jul 29 2020, 11:31 AM · Restricted Project, Restricted Project, Restricted Project
davidxl accepted D84865: [PGO] Remove insignificant function hash values from some tests..

lgtm

Jul 29 2020, 9:49 AM · Restricted Project

Jul 28 2020

davidxl added a comment to D84782: [PGO] Include the mem ops into the function hash..

changes like in llvm/test/Transforms/PGOProfile/PR41279.ll etc can be independently committed.

Jul 28 2020, 5:26 PM · Restricted Project, Restricted Project, Restricted Project
davidxl added a comment to D84782: [PGO] Include the mem ops into the function hash..

Can you split out the test changes and commit it separately?

Jul 28 2020, 4:56 PM · Restricted Project, Restricted Project, Restricted Project
davidxl added a comment to D84782: [PGO] Include the mem ops into the function hash..

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).

Jul 28 2020, 4:13 PM · Restricted Project, Restricted Project, Restricted Project
davidxl added inline comments to D84685: [llvm][NFC] TensorSpec abstraction for ML evaluator.
Jul 28 2020, 3:43 PM · Restricted Project
davidxl added a comment to D84782: [PGO] Include the mem ops into the function hash..

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.

Jul 28 2020, 2:58 PM · Restricted Project, Restricted Project, Restricted Project
davidxl added a comment to D84782: [PGO] Include the mem ops into the function hash..

Rong has pointed out that having 2 bits may not be enough -- also instead of using | operator, add is better.

Jul 28 2020, 12:01 PM · Restricted Project, Restricted Project, Restricted Project
davidxl accepted D84766: Revert D68041 "[PGO] Don't group COMDAT variables for compiler generated profile variables in ELF".

(probably better hold on the patch until after the timeout issue is resolved)

Jul 28 2020, 10:34 AM · Restricted Project
davidxl added a comment to D84766: Revert D68041 "[PGO] Don't group COMDAT variables for compiler generated profile variables in ELF".

Cost: one or two additional section headers (.group section(s)): 64 or 128 bytes on ELF64.

Jul 28 2020, 10:13 AM · Restricted Project
davidxl accepted D84427: [llvm][NFC] refactor setBlockFrequency for clarity..

thanks for the cleanup

Jul 28 2020, 9:59 AM · Restricted Project
davidxl added a comment to D84766: Revert D68041 "[PGO] Don't group COMDAT variables for compiler generated profile variables in ELF".

thanks. Can you update the summary of the revert patch -- I think the first paragraph does not belong.

Jul 28 2020, 9:45 AM · Restricted Project

Jul 27 2020

davidxl added a comment to D84723: [PGO] Move __profc_ and __profvp_ from their own comdat groups to __profd_'s comdat group.

I think it is better to revert https://reviews.llvm.org/D68041 first with reasons listed, and then follow up with an improvement patch to reduce strtab usage.

Jul 27 2020, 10:50 PM · Restricted Project
davidxl added a comment to D84427: [llvm][NFC] refactor setBlockFrequency for clarity..

Perhaps also keep setBlockFreq as a lower level interface? I am surprised that edge splitting is the only one user of it.

Jul 27 2020, 4:44 PM · Restricted Project
davidxl accepted D81981: [PGO] Supplement PGO profile with Sample profile.

lgtm

Jul 27 2020, 10:59 AM · Restricted Project
davidxl accepted D84514: [BPI][NFC] Consolidate code to deal with SCCs under a dedicated data structure..

lgtm

Jul 27 2020, 10:58 AM · Restricted Project
davidxl added inline comments to D81981: [PGO] Supplement PGO profile with Sample profile.
Jul 27 2020, 9:22 AM · Restricted Project

Jul 24 2020

davidxl added inline comments to D84514: [BPI][NFC] Consolidate code to deal with SCCs under a dedicated data structure..
Jul 24 2020, 10:07 AM · Restricted Project
davidxl accepted D84378: [PGO] Fix incorrect function entry count.

lgtm

Jul 24 2020, 9:49 AM · Restricted Project

Jul 23 2020

davidxl added inline comments to D84378: [PGO] Fix incorrect function entry count.
Jul 23 2020, 4:55 PM · Restricted Project
davidxl added inline comments to D84378: [PGO] Fix incorrect function entry count.
Jul 23 2020, 10:25 AM · Restricted Project
davidxl added inline comments to D84378: [PGO] Fix incorrect function entry count.
Jul 23 2020, 9:53 AM · Restricted Project
davidxl accepted D84379: PGO][InstrProf] Do not promote count if the exit blocks contains ret instruction.

lgtm

Jul 23 2020, 9:19 AM · Restricted Project
davidxl added inline comments to D84378: [PGO] Fix incorrect function entry count.
Jul 23 2020, 9:02 AM · Restricted Project

Jul 22 2020

davidxl added a comment to D81981: [PGO] Supplement PGO profile with Sample profile.

Inlining won't be helped unless there is a hot callsite to the all-zero count function -- but this should not exist. I think the major performance hit comes from 1) text.unlikely which may not be mlocked; and 2) all unbiased branches due to zero weights. So doing this depending it on entry count existence is fine, but we still to teach PGOUse to drop the body. I think a simpler design would be

Jul 22 2020, 3:57 PM · Restricted Project
davidxl added a comment to D81981: [PGO] Supplement PGO profile with Sample profile.

The PGOUse pass can choose not to annotate any branches with total weights == 0. Now the question becomes how do we tell PGOUse pass whether the entry should be set to 0 or leave it not set. There are two ways to do it (to signal it is not really cold, but unknown):

Jul 22 2020, 2:53 PM · Restricted Project
davidxl added a comment to D79485: [BPI] Improve static heuristics for "cold" paths..

The PGOUse pass can choose not to annotate any branches with total weights == 0. Now the question becomes how do we tell PGOUse pass whether the entry should be set to 0 or leave it not set. There are two ways to do it (to signal it is not really cold, but unknown):

  1. Remove the function from the indexed format profile;
  2. set all counts to some sentinel value such as -1.
Jul 22 2020, 2:51 PM · Restricted Project
davidxl added a comment to D81981: [PGO] Supplement PGO profile with Sample profile.

If we don't need to handle all zero cases using scaling, we can remove the dependency on the always entry patch.

Jul 22 2020, 2:18 PM · Restricted Project