Skip to content

Commit 3fe9333

Browse files
authored
Merge pull request swiftlang#26774 from brentdax/raise-the-profile
[IRGen] Mix -profile-generate into module hash
2 parents 3256392 + 8fa1ee1 commit 3fe9333

File tree

5 files changed

+216
-62
lines changed

5 files changed

+216
-62
lines changed

include/swift/AST/IRGenOptions.h

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
// FIXME: This include is just for llvm::SanitizerCoverageOptions. We should
2727
// split the header upstream so we don't include so much.
2828
#include "llvm/Transforms/Instrumentation.h"
29+
#include "llvm/Support/raw_ostream.h"
2930
#include "llvm/Support/VersionTuple.h"
3031
#include <string>
3132
#include <vector>
@@ -253,13 +254,18 @@ class IRGenOptions {
253254
SanitizeCoverage(llvm::SanitizerCoverageOptions()),
254255
TypeInfoFilter(TypeInfoDumpFilter::All) {}
255256

256-
// Get a hash of all options which influence the llvm compilation but are not
257-
// reflected in the llvm module itself.
258-
unsigned getLLVMCodeGenOptionsHash() {
259-
unsigned Hash = (unsigned)OptMode;
260-
Hash = (Hash << 1) | DisableLLVMOptzns;
261-
Hash = (Hash << 1) | DisableSwiftSpecificLLVMOptzns;
262-
return Hash;
257+
/// Appends to \p os an arbitrary string representing all options which
258+
/// influence the llvm compilation but are not reflected in the llvm module
259+
/// itself.
260+
void writeLLVMCodeGenOptionsTo(llvm::raw_ostream &os) {
261+
// We put a letter between each value simply to keep them from running into
262+
// one another. There might be a vague correspondence between meaning and
263+
// letter, but don't sweat it.
264+
os << 'O' << (unsigned)OptMode
265+
<< 'd' << DisableLLVMOptzns
266+
<< 'D' << DisableSwiftSpecificLLVMOptzns
267+
<< 'p' << GenerateProfile
268+
<< 's' << Sanitizers.toRaw();
263269
}
264270

265271
/// Should LLVM IR value names be emitted and preserved?

include/swift/Basic/FileSystem.h

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,26 @@ namespace swift {
5656
std::error_code moveFileIfDifferent(const llvm::Twine &source,
5757
const llvm::Twine &destination);
5858

59+
enum class FileDifference : uint8_t {
60+
/// The source and destination paths refer to the exact same file.
61+
IdenticalFile,
62+
/// The source and destination paths refer to separate files with identical
63+
/// contents.
64+
SameContents,
65+
/// The source and destination paths refer to separate files with different
66+
/// contents.
67+
DifferentContents
68+
};
69+
70+
/// Compares the files at \p source and \p destination to determine if they
71+
/// are the exact same files, different files with the same contents, or
72+
/// different files with different contents. If \p allowDestinationErrors is
73+
/// set, file system errors relating to the \p destination file return a
74+
/// \c DifferentFile result, rather than an error.
75+
llvm::ErrorOr<FileDifference>
76+
areFilesDifferent(const llvm::Twine &source, const llvm::Twine &destination,
77+
bool allowDestinationErrors);
78+
5979
namespace vfs {
6080
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
6181
getFileOrSTDIN(llvm::vfs::FileSystem &FS,

lib/Basic/FileSystem.cpp

Lines changed: 75 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -163,13 +163,14 @@ std::error_code swift::atomicallyWritingToFile(
163163
return swift::moveFileIfDifferent(temporaryPath.getValue(), outputPath);
164164
}
165165

166-
std::error_code swift::moveFileIfDifferent(const llvm::Twine &source,
167-
const llvm::Twine &destination) {
166+
llvm::ErrorOr<FileDifference>
167+
swift::areFilesDifferent(const llvm::Twine &source,
168+
const llvm::Twine &destination,
169+
bool allowDestinationErrors) {
168170
namespace fs = llvm::sys::fs;
169171

170-
// First check for a self-move.
171172
if (fs::equivalent(source, destination))
172-
return std::error_code();
173+
return FileDifference::IdenticalFile;
173174

174175
OpenFileRAII sourceFile;
175176
fs::file_status sourceStatus;
@@ -182,45 +183,80 @@ std::error_code swift::moveFileIfDifferent(const llvm::Twine &source,
182183
return error;
183184
}
184185

186+
/// Converts an error from the destination file into either an error or
187+
/// DifferentContents return, depending on `allowDestinationErrors`.
188+
auto convertDestinationError = [=](std::error_code error) ->
189+
llvm::ErrorOr<FileDifference> {
190+
if (allowDestinationErrors)
191+
return FileDifference::DifferentContents;
192+
return error;
193+
};
194+
185195
OpenFileRAII destFile;
186196
fs::file_status destStatus;
187-
bool couldReadDest = !fs::openFileForRead(destination, destFile.fd);
188-
if (couldReadDest)
189-
couldReadDest = !fs::status(destFile.fd, destStatus);
190-
191-
// If we could read the destination file, and it matches the source file in
192-
// size, they may be the same. Do an actual comparison of the contents.
193-
if (couldReadDest && sourceStatus.getSize() == destStatus.getSize()) {
194-
uint64_t size = sourceStatus.getSize();
195-
bool same = false;
196-
if (size == 0) {
197-
same = true;
198-
} else {
199-
std::error_code sourceRegionErr;
200-
fs::mapped_file_region sourceRegion(sourceFile.fd,
201-
fs::mapped_file_region::readonly,
202-
size, 0, sourceRegionErr);
203-
if (sourceRegionErr)
204-
return sourceRegionErr;
205-
206-
std::error_code destRegionErr;
207-
fs::mapped_file_region destRegion(destFile.fd,
208-
fs::mapped_file_region::readonly,
209-
size, 0, destRegionErr);
210-
211-
if (!destRegionErr) {
212-
same = (0 == memcmp(sourceRegion.const_data(), destRegion.const_data(),
213-
size));
214-
}
215-
}
216-
217-
// If the file contents are the same, we are done. Just delete the source.
218-
if (same)
219-
return fs::remove(source);
197+
if (std::error_code error = fs::openFileForRead(destination, destFile.fd)) {
198+
// If we can't open the destination file, fail in the specified fashion.
199+
return convertDestinationError(error);
220200
}
201+
if (std::error_code error = fs::status(destFile.fd, destStatus)) {
202+
// If we can't open the destination file, fail in the specified fashion.
203+
return convertDestinationError(error);
204+
}
205+
206+
uint64_t size = sourceStatus.getSize();
207+
if (size != destStatus.getSize())
208+
// If the files are different sizes, they must be different.
209+
return FileDifference::DifferentContents;
210+
if (size == 0)
211+
// If both files are zero size, they must be the same.
212+
return FileDifference::SameContents;
213+
214+
// The two files match in size, so we have to compare the bytes to determine
215+
// if they're the same.
216+
std::error_code sourceRegionErr;
217+
fs::mapped_file_region sourceRegion(sourceFile.fd,
218+
fs::mapped_file_region::readonly,
219+
size, 0, sourceRegionErr);
220+
if (sourceRegionErr)
221+
return sourceRegionErr;
222+
223+
std::error_code destRegionErr;
224+
fs::mapped_file_region destRegion(destFile.fd,
225+
fs::mapped_file_region::readonly,
226+
size, 0, destRegionErr);
227+
228+
if (destRegionErr)
229+
return convertDestinationError(destRegionErr);
230+
231+
if (0 == memcmp(sourceRegion.const_data(), destRegion.const_data(), size))
232+
return FileDifference::SameContents;
233+
234+
return FileDifference::DifferentContents;
235+
}
236+
237+
std::error_code swift::moveFileIfDifferent(const llvm::Twine &source,
238+
const llvm::Twine &destination) {
239+
namespace fs = llvm::sys::fs;
240+
241+
auto result = areFilesDifferent(source, destination,
242+
/*allowDestinationErrors=*/true);
221243

222-
// If we get here, we weren't able to prove that the files are the same.
223-
return fs::rename(source, destination);
244+
if (!result)
245+
return result.getError();
246+
247+
switch (*result) {
248+
case FileDifference::IdenticalFile:
249+
// Do nothing for a self-move.
250+
return std::error_code();
251+
252+
case FileDifference::SameContents:
253+
// Files are identical; remove the source file.
254+
return fs::remove(source);
255+
256+
case FileDifference::DifferentContents:
257+
// Files are different; overwrite the destination file.
258+
return fs::rename(source, destination);
259+
}
224260
}
225261

226262
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>

lib/IRGen/IRGen.cpp

Lines changed: 89 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -349,7 +349,7 @@ static void getHashOfModule(MD5::MD5Result &Result, IRGenOptions &Opts,
349349

350350
// Add all options which influence the llvm compilation but are not yet
351351
// reflected in the llvm module itself.
352-
HashStream << Opts.getLLVMCodeGenOptionsHash();
352+
Opts.writeLLVMCodeGenOptionsTo(HashStream);
353353

354354
HashStream.final(Result);
355355
}
@@ -427,6 +427,22 @@ static void countStatsPostIRGen(UnifiedStatsReporter &Stats,
427427
}
428428
}
429429

430+
template<typename ...ArgTypes>
431+
void
432+
diagnoseSync(DiagnosticEngine *Diags, llvm::sys::Mutex *DiagMutex,
433+
SourceLoc Loc, Diag<ArgTypes...> ID,
434+
typename swift::detail::PassArgument<ArgTypes>::type... Args) {
435+
if (!Diags)
436+
return;
437+
if (DiagMutex)
438+
DiagMutex->lock();
439+
440+
Diags->diagnose(Loc, ID, std::move(Args)...);
441+
442+
if (DiagMutex)
443+
DiagMutex->unlock();
444+
}
445+
430446
/// Run the LLVM passes. In multi-threaded compilation this will be done for
431447
/// multiple LLVM modules in parallel.
432448
bool swift::performLLVM(IRGenOptions &Opts, DiagnosticEngine *Diags,
@@ -437,6 +453,18 @@ bool swift::performLLVM(IRGenOptions &Opts, DiagnosticEngine *Diags,
437453
const version::Version &effectiveLanguageVersion,
438454
StringRef OutputFilename,
439455
UnifiedStatsReporter *Stats) {
456+
#ifndef NDEBUG
457+
// To check that we only skip generating code when it would have no effect, in
458+
// assertion builds we still generate the code, but write it into a temporary
459+
// file that we compare to the original file.
460+
461+
/// The OutputFilename originally passed to us, if we are generating code for
462+
/// an assertion. Empty if not.
463+
StringRef OriginalOutputFilename = "";
464+
/// Scratch buffer for temporary file's name.
465+
SmallString<64> AssertScratch;
466+
#endif
467+
440468
if (Opts.UseIncrementalLLVMCodeGen && HashGlobal) {
441469
// Check if we can skip the llvm part of the compilation if we have an
442470
// existing object file which was generated from the same llvm IR.
@@ -458,7 +486,24 @@ bool swift::performLLVM(IRGenOptions &Opts, DiagnosticEngine *Diags,
458486
!Opts.PrintInlineTree &&
459487
!needsRecompile(OutputFilename, HashData, HashGlobal, DiagMutex)) {
460488
// The llvm IR did not change. We don't need to re-create the object file.
489+
#ifdef NDEBUG
461490
return false;
491+
#else
492+
// ...but we're in an asserts build, so we want to check that assumption.
493+
auto AssertSuffix = llvm::sys::path::filename(OutputFilename);
494+
495+
auto EC = llvm::sys::fs::createTemporaryFile("assert", AssertSuffix,
496+
AssertScratch);
497+
if (EC) {
498+
diagnoseSync(Diags, DiagMutex,
499+
SourceLoc(), diag::error_opening_output,
500+
AssertScratch, EC.message());
501+
return true;
502+
}
503+
504+
OriginalOutputFilename = OutputFilename;
505+
OutputFilename = AssertScratch;
506+
#endif
462507
}
463508

464509
// Store the hash in the global variable so that it is written into the
@@ -476,14 +521,9 @@ bool swift::performLLVM(IRGenOptions &Opts, DiagnosticEngine *Diags,
476521
RawOS.emplace(OutputFilename, EC, OSFlags);
477522

478523
if (RawOS->has_error() || EC) {
479-
if (Diags) {
480-
if (DiagMutex)
481-
DiagMutex->lock();
482-
Diags->diagnose(SourceLoc(), diag::error_opening_output,
483-
OutputFilename, EC.message());
484-
if (DiagMutex)
485-
DiagMutex->unlock();
486-
}
524+
diagnoseSync(Diags, DiagMutex,
525+
SourceLoc(), diag::error_opening_output,
526+
OutputFilename, EC.message());
487527
RawOS->clear_error();
488528
return true;
489529
}
@@ -525,13 +565,8 @@ bool swift::performLLVM(IRGenOptions &Opts, DiagnosticEngine *Diags,
525565
bool fail = TargetMachine->addPassesToEmitFile(EmitPasses, *RawOS, nullptr,
526566
FileType, !Opts.Verify);
527567
if (fail) {
528-
if (Diags) {
529-
if (DiagMutex)
530-
DiagMutex->lock();
531-
Diags->diagnose(SourceLoc(), diag::error_codegen_init_fail);
532-
if (DiagMutex)
533-
DiagMutex->unlock();
534-
}
568+
diagnoseSync(Diags, DiagMutex,
569+
SourceLoc(), diag::error_codegen_init_fail);
535570
return true;
536571
}
537572
break;
@@ -555,6 +590,44 @@ bool swift::performLLVM(IRGenOptions &Opts, DiagnosticEngine *Diags,
555590
if (DiagMutex)
556591
DiagMutex->unlock();
557592
}
593+
594+
#ifndef NDEBUG
595+
if (!OriginalOutputFilename.empty()) {
596+
// We're done changing the file; make sure it's saved before we compare.
597+
RawOS->close();
598+
599+
auto result =
600+
swift::areFilesDifferent(OutputFilename, OriginalOutputFilename,
601+
/*allowDestinationErrors=*/false);
602+
603+
if (!result)
604+
// File system error.
605+
llvm::report_fatal_error(
606+
Twine("Error comparing files: ") + result.getError().message()
607+
);
608+
609+
switch (*result) {
610+
case FileDifference::DifferentContents:
611+
llvm::report_fatal_error(
612+
"Swift skipped an LLVM compile that would have changed output; pass "
613+
"-Xfrontend -disable-incremental-llvm-codegen to work around this bug"
614+
);
615+
// Note for future debuggers: If you see this error, either you changed
616+
// LLVM and need to clean your build folder to rebuild everything with it,
617+
// or IRGenOptions::writeLLVMCodeGenOptionsTo() doesn't account for a flag
618+
// that changed LLVM's output between this compile and the previous one.
619+
620+
case FileDifference::SameContents:
621+
// Removing the file is best-effort.
622+
(void)llvm::sys::fs::remove(OutputFilename);
623+
break;
624+
625+
case FileDifference::IdenticalFile:
626+
llvm_unreachable("one of these should be a temporary file");
627+
}
628+
}
629+
#endif
630+
558631
return false;
559632
}
560633

test/IRGen/module_hash.swift

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,25 @@
3131
// CHECK: test.o: MD5=[[TEST3_MD5:[0-9a-f]+]]
3232
// CHECK: test.o: prev MD5=[[TEST2_MD5]] recompiling
3333

34+
// RUN: echo "empty file" >>%t/log
35+
// RUN: %target-swift-frontend -c -primary-file %S/../Inputs/empty.swift -module-name=empty -o %t/empty.o -Xllvm -debug-only=irgen 2>>%t/log
36+
// RUN: echo "empty file with profiling" >>%t/log
37+
// RUN: %target-swift-frontend -c -primary-file %S/../Inputs/empty.swift -module-name=empty -o %t/empty.o -Xllvm -debug-only=irgen -profile-generate 2>>%t/log
38+
39+
// CHECK-LABEL: empty file
40+
// CHECK: empty.o: MD5=[[EMPTY_MD5:[0-9a-f]+]]
41+
// CHECK-LABEL: empty file with profiling
42+
// CHECK: empty.o: prev MD5=[[EMPTY_MD5]] recompiling
43+
44+
// RUN: echo "empty file" >>%t/log
45+
// RUN: %target-swift-frontend -c -primary-file %S/../Inputs/empty.swift -module-name=empty -o %t/empty.o -Xllvm -debug-only=irgen 2>>%t/log
46+
// RUN: echo "empty file with fuzzer" >>%t/log
47+
// RUN: %target-swift-frontend -c -primary-file %S/../Inputs/empty.swift -module-name=empty -o %t/empty.o -Xllvm -debug-only=irgen -sanitize=fuzzer 2>>%t/log
48+
49+
// CHECK-LABEL: empty file
50+
// CHECK: empty.o: MD5=[[EMPTY_MD5_2:[0-9a-f]+]]
51+
// CHECK-LABEL: empty file with fuzzer
52+
// CHECK: empty.o: prev MD5=[[EMPTY_MD5_2]] recompiling
3453

3554
// Test with multiple output files
3655

0 commit comments

Comments
 (0)