This is an archive of the discontinued LLVM Phabricator instance.

[CUDA][HIP] Add -fuse-cuid
ClosedPublic

Authored by yaxunl on Jan 19 2021, 3:20 PM.

Details

Summary

This patch added a distinct CUID for each input file, which is represented by InputAction.
clang initially creates an InputAction for each input file for the host compilation. In CUDA/HIP action
builder, each InputAction is given a CUID and cloned for each GPU arch, and the CUID is also cloned. In this way,
we guarantee the corresponding device and host compilation for the same file shared the
same CUID. On the other hand, different compilation units have different CUID.

-fuse-cuid=random|hash|none is added to control the method to generate CUID. The default
is hash. -cuid=X is also added to specify CUID explicitly, which overrides -fuse-cuid.

This patch is separated from https://reviews.llvm.org/D85223

Diff Detail

Event Timeline

yaxunl created this revision.Jan 19 2021, 3:20 PM

@jdoerfert this may be a candidate for deriving names of extracted target regions

clang/lib/Frontend/CompilerInvocation.cpp
2746

Why this limitation? In particular I can imagine people using an id based on a filesystem location, which introduces /\:. and possibly other characters.

tra added inline comments.Jan 20 2021, 10:47 AM
clang/lib/Frontend/CompilerInvocation.cpp
2746

I guess one of the reasons is that CUID is used to create unique externally visible symbol names, so the charset is sort of the lowest common denominator that could be directly used in the symbol name.

We could work around that by accepting an arbitrary string as a CUID and then using its hash to create unique stable suffix.

yaxunl marked 2 inline comments as done.Jan 21 2021, 1:37 PM
yaxunl added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
2746

will remove the check and use hash for suffix.

yaxunl updated this revision to Diff 322020.Feb 7 2021, 8:56 PM
yaxunl marked an inline comment as done.

Revised by Jon and Artem's comments. Removed check of CUID value.

tra accepted this revision.Feb 8 2021, 2:45 PM

Few test nits. LGTM in principle.

clang/test/Driver/hip-cuid.hip
99

Is it necessary? The next 'RUN' command would overwrite the temp file anyways.
Also, you don't have to call all temporary files %t.out. You could use %t-1.out, etc and leave the cleanup to the test driver.

101–103

Putting RUN down in the test file makes them easy to miss. Maybe these should be in their own hip-cuid-rdc.hip test structured in a conventional way -- RUN lines first, followed by checks.

This revision is now accepted and ready to land.Feb 8 2021, 2:45 PM
yaxunl marked 2 inline comments as done.Feb 8 2021, 6:41 PM
yaxunl added inline comments.
clang/test/Driver/hip-cuid.hip
99

will remove rm.

101–103

will split the test

This revision was automatically updated to reflect the committed changes.
yaxunl marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 7:28 PM

Should -cuid= be --cuid=?

Should -cuid= be --cuid=?

For the existing clang options, there seem to be no rule or convention whether to use - or --. For example,

-rtlib=<value>          Compiler runtime library to use
-save-stats=<value>     Save llvm statistics.
-save-temps=<value>     Save intermediate compilation results.
-std=<value>            Language standard to compile for
-stdlib=<value>         C++ standard library to use
-struct-layout=<value>  Run the Structure Peeling passes
-struct-peel-mem-block-size=<value>
-struct-peel-ptr-size=<value>
-sycl-std=<value>       SYCL language standard to compile for.

Here I choose -cuid= since it is shorter.