This is an archive of the discontinued LLVM Phabricator instance.

[HIP] Fix temporary files
ClosedPublic

Authored by yaxunl on Mar 7 2023, 8:10 AM.

Details

Summary

Currently HIP toolchain uses Driver::GetTemporaryDirectory to
create a temporary directory for some temporary files during
compilation. The temporary directories are not automatically
deleted after compilation. This slows down compilation
on Windows.

Switch to use GetTemporaryPath which only creates temporay
files which will be deleted automatically.

Keep the original input file name convention for Darwin host
toolchain since it is needed for deterministic binary
(https://reviews.llvm.org/D111269)

Diff Detail

Event Timeline

yaxunl created this revision.Mar 7 2023, 8:10 AM
Herald added a project: Restricted Project. · View Herald Transcript
yaxunl requested review of this revision.Mar 7 2023, 8:10 AM
tra added a comment.Mar 7 2023, 10:15 AM

LGTM, but we should probably get someone familiar with macos to chime in, just in case there may be some reason behind macos using temp directories here.

This change is OK for MacOS as lipo does not requires specific

I'm curious why lipo has been singled out. Is that the only use case that ends up using this path?

clang/lib/Driver/Driver.cpp
5568–5580

This could now be shortened to:

TmpName = (MultipleArchs && !BoundArch.empty()) 
    ? GetTemporaryPath((Twine(Prefix) + "-" + BoundArch).str(), Suffix)
    : GetTemporaryPath(Prefix, Suffix);
yaxunl added a comment.Mar 7 2023, 6:55 PM

LGTM, but we should probably get someone familiar with macos to chime in, just in case there may be some reason behind macos using temp directories here.

This change is OK for MacOS as lipo does not requires specific

I'm curious why lipo has been singled out. Is that the only use case that ends up using this path?

AFAIK clang/llvm tools and lld does not depend on input file names, but I am not so sure about lipo, that's why I checked lipo man page.

However, it seems the previous code using GetTemporaryDirectory was intentional (https://reviews.llvm.org/D111269). My guess is that lipo using some hash of input file name in the generated binary, GetTemporaryPath will make the generated binary non-deterministic.

It seems I need to limit this change to HIP as HIP uses clang-offload-bundler which generates binary not depending on input file names.

yaxunl updated this revision to Diff 503218.Mar 7 2023, 8:26 PM
yaxunl edited the summary of this revision. (Show Details)
yaxunl added a reviewer: keith.
tra added a comment.Mar 8 2023, 12:36 PM

However, it seems the previous code using GetTemporaryDirectory was intentional (https://reviews.llvm.org/D111269). My guess is that lipo using some hash of input file name in the generated binary, GetTemporaryPath will make the generated binary non-deterministic.

Hmm. This is unfortunate.

clang/include/clang/Driver/Driver.h
642

I don't think that commingling offloading with something as generic as temp file creation is a good choice here.

I think we may want to coalesce both MultipleArchs and OFK into a more generic Mode argument which would have knobs for UNIQUE_DIRECTORY and MULTIPLE_ARCH.
I suspect MULTIPLE_ARCH may be redundant -- we may make BoundArch an optional<StringRef> which would be a somewhat cleaner way to indicate whether file. Or just rely on BoundArch.empty().

Then figure out the right mode at the call site, where we'd have more context for what we do and why without having to propagate that knowledge to CreateTempFile.

So, roughly something like this:

const char *CreateTempFile(Compilation &C, StringRef Prefix, StringRef Suffix,
                             StringRef BoundArch = {},
                             bool UniqueDirectory = false) const;

....
    // MacOS must have stable file name, because reasons.
    bool NeedsUniqueDir = (OFK == Action::OFK_None || OFK == Action::OFK_Host) && Triple.isOSDarwin());
    return CreateTempFile(C, Split.first, Suffix, MultipleArchs ? BoundArch : "", NeedsUniqueDir);
yaxunl marked an inline comment as done.Mar 8 2023, 1:47 PM
yaxunl added inline comments.
clang/include/clang/Driver/Driver.h
642

will do

yaxunl updated this revision to Diff 503504.Mar 8 2023, 1:48 PM
yaxunl marked an inline comment as done.

revised by Artem's comments

tra accepted this revision.Mar 8 2023, 1:54 PM
tra added inline comments.
clang/include/clang/Driver/Driver.h
630–638

The comment should probably be updated to reflect the effect of the optional arguments.

This revision is now accepted and ready to land.Mar 8 2023, 1:54 PM
yaxunl marked an inline comment as done.Mar 9 2023, 6:56 AM
yaxunl added inline comments.
clang/include/clang/Driver/Driver.h
630–638

will do

yaxunl updated this revision to Diff 503826.Mar 9 2023, 10:05 AM
yaxunl marked an inline comment as done.

fix comments and tests

yaxunl updated this revision to Diff 503892.Mar 9 2023, 12:52 PM

fix tests

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 6:42 PM