Page MenuHomePhabricator

Northbadge (Jin Xin Ng)
User

Projects

User does not belong to any projects.

User Details

User Since
May 27 2022, 11:20 AM (11 w, 1 d)

Recent Activity

Jul 11 2022

Northbadge added a comment to D127779: [LTO][ELF] Add --lto-exit-on option.

An update on this: I found a different use for --lto-exit-on (well, the original intended use: in MLGO training cycles which run >1M times, which I thought wouldn't have worked initially). I'm trial-ing to make sure it's doing what I'm expecting so we can probably put this patch on hold for a week or two, it isn't super-critical at the moment. I'll report back then (+details) and see how it goes from there

Jul 11 2022, 5:03 PM · Restricted Project, Restricted Project

Jul 7 2022

Northbadge added a comment to D127779: [LTO][ELF] Add --lto-exit-on option.

I can see that fine-grained --save-temps=xxx is useful, but the utility of --lto-exit-on is unclear. Why is the overhead considered so high that an option needs to be added?

Jul 7 2022, 10:08 PM · Restricted Project, Restricted Project
Northbadge added a comment to D127779: [LTO][ELF] Add --lto-exit-on option.

The summary does not mention the motivation adding --lto-exit-on. Why is useful enough to have such an option?

Jul 7 2022, 3:56 PM · Restricted Project, Restricted Project
Northbadge updated the summary of D127779: [LTO][ELF] Add --lto-exit-on option.
Jul 7 2022, 3:55 PM · Restricted Project, Restricted Project

Jul 6 2022

Northbadge committed rG65001f5777db: [LTO][ELF] Add selective --save-temps= option (authored by Northbadge).
[LTO][ELF] Add selective --save-temps= option
Jul 6 2022, 10:07 AM · Restricted Project, Restricted Project
Northbadge closed D127778: [LTO][ELF] Add selective --save-temps= option.
Jul 6 2022, 10:07 AM · Restricted Project, Restricted Project

Jul 1 2022

Northbadge updated the diff for D127779: [LTO][ELF] Add --lto-exit-on option.

Fix test case from missing word

Jul 1 2022, 8:47 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127779: [LTO][ELF] Add --lto-exit-on option.

Missed a word, and braces

Jul 1 2022, 7:49 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127779: [LTO][ELF] Add --lto-exit-on option.

Bring back deleted options, updated to be consistent with parent diff, added llvm-lto2 tests.

Jul 1 2022, 7:45 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Missed semicolons.

Jul 1 2022, 3:52 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Walked back the lit change for cross-platform escape char (didn't work). Disabled the tests on Windows instead, let me know if I should avoid doing that. I'll probably land this after the long weekend. Thanks for all the feedback!

Jul 1 2022, 3:04 PM · Restricted Project, Restricted Project
Northbadge added inline comments to D127778: [LTO][ELF] Add selective --save-temps= option.
Jul 1 2022, 11:55 AM · Restricted Project, Restricted Project
Northbadge added inline comments to D127778: [LTO][ELF] Add selective --save-temps= option.
Jul 1 2022, 11:54 AM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Resolved inline comments; Also having buildbots test a potential fix for escaping characters on Windows (will split it into a separate patch if it works)

Jul 1 2022, 11:50 AM · Restricted Project, Restricted Project

Jun 30 2022

Northbadge added inline comments to D127778: [LTO][ELF] Add selective --save-temps= option.
Jun 30 2022, 12:56 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Update to use DenseSet, cleaned up tests and arg parsing. Thanks for the feedback!

Jun 30 2022, 10:12 AM · Restricted Project, Restricted Project

Jun 29 2022

Northbadge added inline comments to D127778: [LTO][ELF] Add selective --save-temps= option.
Jun 29 2022, 6:30 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Removed post* prefix, use DenseSet<StringRef> to store args

Jun 29 2022, 6:29 PM · Restricted Project, Restricted Project
Northbadge added a comment to D127778: [LTO][ELF] Add selective --save-temps= option.

Ping on the llvm-lto2 change.

Jun 29 2022, 5:05 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Added LLVM-side tests, modified llvm-lto2, fixed some capitalization and typos in tests.

Jun 29 2022, 5:03 PM · Restricted Project, Restricted Project
Northbadge committed rGf702c7bb9ef7: [ThinLTO][test] Add tests for emitting files in-process (authored by Northbadge).
[ThinLTO][test] Add tests for emitting files in-process
Jun 29 2022, 2:43 PM · Restricted Project, Restricted Project
Northbadge closed D128771: [ThinLTO][test] Add tests for emitting files in-process.
Jun 29 2022, 2:43 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D128771: [ThinLTO][test] Add tests for emitting files in-process.

Thanks for the review! Added support for -thinlto-emit-imports with -thinlto-distributed-indexes

Jun 29 2022, 1:44 PM · Restricted Project, Restricted Project

Jun 28 2022

Northbadge requested review of D128771: [ThinLTO][test] Add tests for emitting files in-process.
Jun 28 2022, 4:44 PM · Restricted Project, Restricted Project

Jun 27 2022

Northbadge added a comment to D127778: [LTO][ELF] Add selective --save-temps= option.

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

This is a good point, and for D127779 as well.

I defer to the both of you on that point. However, this patch and D127779 are more of an extension of --save-temps (in my opinion), and as such the tests are predicated on --save-temps being correct. I just assumed --save-temps was properly tested to begin with, but it seems to have only been tested within ELF/lto on a basic level. Would it be sufficient (for these 2 patches) to write llvm side tests for --save-temps as a separate patch?

It's not the --save-temps option that should be tested by llvm-lto2, but rather the core LTO changes (which are linker independent) being introduced in the patches. Typically when we add functionality to LTO we test it independently of any linker, via llvm-lto2, in one or both of the test directories @MaskRay mentioned above, then also add tests for whatever linker is utilizing the LTO support.

I missed this in D127777, which should ideally also have some llvm-lto2 based testing. E.g. see the -thinlto-distributed-indexes option in llvm/tools/llvm-lto2/llvm-lto2.cpp which is testing the thinlto-index-only functionality (llvm-lto2.cpp also has a -save-temps option). There are a bunch of tests under llvm/test/ThinLTO/X86/ that utilize these options (and end up testing the related LTO functionality in the process).

Jun 27 2022, 9:08 AM · Restricted Project, Restricted Project

Jun 24 2022

Northbadge added a comment to D127778: [LTO][ELF] Add selective --save-temps= option.

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

This is a good point, and for D127779 as well.

Jun 24 2022, 4:48 PM · Restricted Project, Restricted Project

Jun 23 2022

Northbadge updated the diff for D127779: [LTO][ELF] Add --lto-exit-on option.

Update test to use subdirectories.

Jun 23 2022, 12:43 PM · Restricted Project, Restricted Project
Northbadge committed rG22f1273357cf: [ThinLTO][ELF] Add --thinlto-emit-index-files option (authored by Northbadge).
[ThinLTO][ELF] Add --thinlto-emit-index-files option
Jun 23 2022, 12:39 PM · Restricted Project, Restricted Project
Northbadge closed D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.
Jun 23 2022, 12:38 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Removed the saveTemps bool, updated test to use subdirectories

Jun 23 2022, 11:32 AM · Restricted Project, Restricted Project
Northbadge updated the diff for D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.

Switch from test to ls. Thanks!

Jun 23 2022, 10:40 AM · Restricted Project, Restricted Project

Jun 22 2022

Northbadge added inline comments to D127778: [LTO][ELF] Add selective --save-temps= option.
Jun 22 2022, 6:15 PM · Restricted Project, Restricted Project
Northbadge added inline comments to D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.
Jun 22 2022, 5:42 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.

Resolved most inline comments

Jun 22 2022, 5:42 PM · Restricted Project, Restricted Project
Northbadge added inline comments to D127779: [LTO][ELF] Add --lto-exit-on option.
Jun 22 2022, 1:03 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127779: [LTO][ELF] Add --lto-exit-on option.

Removed support for --lto-exit-on={preopt,postinternalize} due to ambiguity when running ThinLTO.

Jun 22 2022, 1:03 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Reworked the test case following feedback, thanks!

Jun 22 2022, 12:38 PM · Restricted Project, Restricted Project
Northbadge added a comment to D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.

Allows ThinLTO indices to be written to disk on-the-fly/as-part-of “normal” linker execution. Previously thinLTO indices could be written via --thinlto-index-only but that would cause the linker to exit early.

Can you state the motivation for not exiting early?

Jun 22 2022, 11:29 AM · Restricted Project, Restricted Project
Northbadge updated the summary of D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.
Jun 22 2022, 11:24 AM · Restricted Project, Restricted Project
Northbadge updated the diff for D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.

Add support for --thinlto-emit-imports-files, and resolved inline comments. Thanks for the reviews!

Jun 22 2022, 11:23 AM · Restricted Project, Restricted Project

Jun 21 2022

Northbadge added inline comments to D127779: [LTO][ELF] Add --lto-exit-on option.
Jun 21 2022, 9:15 PM · Restricted Project, Restricted Project
Northbadge added a comment to D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.

gentle reminder, thanks!

Jun 21 2022, 11:48 AM · Restricted Project, Restricted Project

Jun 15 2022

Northbadge updated the diff for D127779: [LTO][ELF] Add --lto-exit-on option.

Resolved inline comments, synced with parent, and removed -find from the test case. Thanks for the reviews!

Jun 15 2022, 3:20 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127778: [LTO][ELF] Add selective --save-temps= option.

Resolved inline comments. Thanks for the review!

Jun 15 2022, 3:19 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127693: [mlgo] Fix accounting for SCC splits.

Actually rebase to upstream main

Jun 15 2022, 10:25 AM · Restricted Project, Restricted Project
Northbadge updated the diff for D127693: [mlgo] Fix accounting for SCC splits.
Jun 15 2022, 10:25 AM · Restricted Project, Restricted Project
Northbadge updated the diff for D127693: [mlgo] Fix accounting for SCC splits.

Rebase to upstream main

Jun 15 2022, 10:14 AM · Restricted Project, Restricted Project

Jun 14 2022

Northbadge updated the diff for D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.

Add missing braces

Jun 14 2022, 2:31 PM · Restricted Project, Restricted Project
Northbadge added inline comments to D127693: [mlgo] Fix accounting for SCC splits.
Jun 14 2022, 2:26 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127693: [mlgo] Fix accounting for SCC splits.

Resolved comments. Thanks for the quick review!

Jun 14 2022, 2:12 PM · Restricted Project, Restricted Project
Northbadge updated the summary of D127779: [LTO][ELF] Add --lto-exit-on option.
Jun 14 2022, 1:04 PM · Restricted Project, Restricted Project
Northbadge requested review of D127779: [LTO][ELF] Add --lto-exit-on option.
Jun 14 2022, 1:01 PM · Restricted Project, Restricted Project
Northbadge requested review of D127778: [LTO][ELF] Add selective --save-temps= option.
Jun 14 2022, 1:00 PM · Restricted Project, Restricted Project
Northbadge requested review of D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.
Jun 14 2022, 12:59 PM · Restricted Project, Restricted Project

Jun 13 2022

Northbadge updated the diff for D127689: [inliner] Add per-SCC-pass InlineAdvisor printing option.

Moved ML test

Jun 13 2022, 2:55 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127689: [inliner] Add per-SCC-pass InlineAdvisor printing option.

Added one more test case.

Jun 13 2022, 2:53 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127693: [mlgo] Fix accounting for SCC splits.

Sync with parent change

Jun 13 2022, 2:45 PM · Restricted Project, Restricted Project
Northbadge retitled D127689: [inliner] Add per-SCC-pass InlineAdvisor printing option from [inliner] Add per-SCC Inline Advisor printing option to [inliner] Add per-SCC-pass InlineAdvisor printing option.
Jun 13 2022, 2:35 PM · Restricted Project, Restricted Project
Northbadge updated the diff for D127689: [inliner] Add per-SCC-pass InlineAdvisor printing option.

Resolve comments and added tests.

Jun 13 2022, 2:33 PM · Restricted Project, Restricted Project
Northbadge requested review of D127693: [mlgo] Fix accounting for SCC splits.
Jun 13 2022, 1:46 PM · Restricted Project, Restricted Project
Northbadge requested review of D127689: [inliner] Add per-SCC-pass InlineAdvisor printing option.
Jun 13 2022, 1:23 PM · Restricted Project, Restricted Project

Jun 7 2022

Northbadge requested review of D127245: [mlgo] Disable accounting upon ForceStop.
Jun 7 2022, 12:55 PM · Restricted Project, Restricted Project