This is an archive of the discontinued LLVM Phabricator instance.

Differential D84569

[ELF] --reproduce should include lto sample profile
ClosedPublic

Authored by christylee on Jul 24 2020, 5:19 PM.

Details

Reviewers
• espindola
MaskRay
ruiu
Commits
rGbd4757cc4e40: [ELF] --reproduce should include lto sample profile

Diff Detail

Event Timeline

christylee created this revision.Jul 24 2020, 5:19 PM
Herald added a reviewer: • espindola. · View Herald TranscriptJul 24 2020, 5:19 PM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, dexonsmith, MaskRay and 3 others. · View Herald Transcript
christylee updated this revision to Diff 280633.Jul 24 2020, 5:21 PM

Removed debug comment.

christylee added a reviewer: ruiu.Jul 24 2020, 5:23 PM
christylee marked an inline comment as done.
christylee added inline comments.
lld/ELF/Driver.cpp
498

At this point config->ltoSampleProfile hasn't been initialized yet so we have to read in the argument value. However, this seems like the most logical place to append extra stuff to the link repro tar.

MaskRay added inline comments.Jul 24 2020, 5:32 PM
lld/test/ELF/reproduce.s
8 ↗(On Diff #280633)

Add the test to reproduce-lto.s

Harbormaster completed remote builds in B65662: Diff 280631.Jul 24 2020, 5:44 PM
Harbormaster completed remote builds in B65665: Diff 280633.Jul 24 2020, 6:14 PM
christylee updated this revision to Diff 280645.Jul 24 2020, 6:30 PM

Moved test case to reproduce-lto.s

Herald added subscribers: steven_wu, hiraditya. · View Herald TranscriptJul 24 2020, 6:30 PM
Harbormaster completed remote builds in B65673: Diff 280645.Jul 24 2020, 7:10 PM
MaskRay added inline comments.Jul 24 2020, 7:18 PM
lld/ELF/Driver.cpp
506

Just move this line above so that we can use config->ltoSampleProfile

lld/test/ELF/reproduce-lto.s
6

-triple=x86_64

7

echo > %t.dir/build1/empty_profile.txt

9

Delete --hash-style=gnu

Use -o /dev/null

christylee marked an inline comment as done.Jul 27 2020, 8:59 AM
christylee added inline comments.
lld/ELF/Driver.cpp
506

If we move config->ltoSampleProfile up to before getReproduceOptions(args), then symbol ordering file, dynamic list, and version script will be consumed before the tar object is created, so they won't be included in the repro.tar. Would it be better if I move this to where we parse LTO options?

MaskRay accepted this revision.Jul 27 2020, 9:10 AM

LGTM.

This revision is now accepted and ready to land.Jul 27 2020, 9:10 AM
christylee updated this revision to Diff 280941.Jul 27 2020, 9:13 AM

Addressed comments, fixed unittest.

Harbormaster completed remote builds in B65845: Diff 280941.Jul 27 2020, 11:09 AM
Closed by commit rGbd4757cc4e40: [ELF] --reproduce should include lto sample profile (authored by christylee, committed by wenlei). · Explain WhyJul 28 2020, 9:48 AM
This revision was automatically updated to reflect the committed changes.
wenlei added a commit: rGbd4757cc4e40: [ELF] --reproduce should include lto sample profile.
wenlei added a subscriber: wenlei.Jul 28 2020, 9:52 AM

Committed on behalf of @christylee while she tries to recover commit access.

Revision Contents

PathSize
lld/
ELF/
3 lines
test/
ELF/
10 lines

Diff 281265

lld/ELF/Driver.cpp

Show First 20 Lines • Show All 489 Lines • ▼ Show 20 Lines if (const char *path = getReproduceOption(args)) {
// Note that --reproduce is a debug option so you can ignore it // Note that --reproduce is a debug option so you can ignore it
// if you are trying to understand the whole picture of the code. // if you are trying to understand the whole picture of the code.
Expected<std::unique_ptr<TarWriter>> errOrWriter = Expected<std::unique_ptr<TarWriter>> errOrWriter =
TarWriter::create(path, path::stem(path)); TarWriter::create(path, path::stem(path));
if (errOrWriter) { if (errOrWriter) {
tar = std::move(*errOrWriter); tar = std::move(*errOrWriter);
tar->append("response.txt", createResponseFile(args)); tar->append("response.txt", createResponseFile(args));
tar->append("version.txt", getLLDVersion() + "\n"); tar->append("version.txt", getLLDVersion() + "\n");
StringRef ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
christyleeAuthorUnsubmitted
Done
ReplyInline Actions

At this point config->ltoSampleProfile hasn't been initialized yet so we have to read in the argument value. However, this seems like the most logical place to append extra stuff to the link repro tar.

christylee: At this point `config->ltoSampleProfile` hasn't been initialized yet so we have to read in the…
if (!ltoSampleProfile.empty())
readFile(ltoSampleProfile);
} else { } else {
error("--reproduce: " + toString(errOrWriter.takeError())); error("--reproduce: " + toString(errOrWriter.takeError()));
} }
} }
readConfigs(args); readConfigs(args);
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Just move this line above so that we can use config->ltoSampleProfile

MaskRay: Just move this line above so that we can use `config->ltoSampleProfile`
christyleeAuthorUnsubmitted
Done
ReplyInline Actions

If we move config->ltoSampleProfile up to before getReproduceOptions(args), then symbol ordering file, dynamic list, and version script will be consumed before the tar object is created, so they won't be included in the repro.tar. Would it be better if I move this to where we parse LTO options?

christylee: If we move `config->ltoSampleProfile` up to before `getReproduceOptions(args)`, then symbol…
// The behavior of -v or --version is a bit strange, but this is // The behavior of -v or --version is a bit strange, but this is
// needed for compatibility with GNU linkers. // needed for compatibility with GNU linkers.
if (args.hasArg(OPT_v) && !args.hasArg(OPT_INPUT)) if (args.hasArg(OPT_v) && !args.hasArg(OPT_INPUT))
return; return;
if (args.hasArg(OPT_version)) if (args.hasArg(OPT_version))
return; return;
▲ Show 20 Lines • Show All 1,653 Lines • Show Last 20 Lines

lld/test/ELF/reproduce-lto.s

  • This file was added.
# REQUIRES: x86
# RUN: rm -rf %t.dir
# RUN: mkdir -p %t.dir/build1
# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.dir/build1/foo.o
# RUN: echo > %t.dir/build1/empty_profile.txt
MaskRayUnsubmitted
Not Done
ReplyInline Actions

-triple=x86_64

MaskRay: `-triple=x86_64`
# RUN: cd %t.dir
MaskRayUnsubmitted
Not Done
ReplyInline Actions

echo > %t.dir/build1/empty_profile.txt

MaskRay: `echo > %t.dir/build1/empty_profile.txt`
# RUN: ld.lld build1/foo.o -o /dev/null --reproduce repro1.tar --lto-sample-profile=%t.dir/build1/empty_profile.txt
# RUN: tar tvf repro1.tar | FileCheck %s
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Delete --hash-style=gnu

Use -o /dev/null

MaskRay: Delete `--hash-style=gnu` Use `-o /dev/null`
# CHECK: repro1/{{.*}}/empty_profile.txt