Page MenuHomePhabricator

[HIP] unbundle bundled preprocessor output
ClosedPublic

Authored by yaxunl on Dec 5 2020, 6:35 AM.

Details

Summary

There is a use case that users want to emit preprocessor
output as file and compile the preprocessor output later
with -x hip-cpp-output.

Clang emits bundled preprocessor output when users
compile with -E for combined host/device compilations.
Clang should be able to compile the bundled preprocessor
output with -x hip-cpp-output. Basically clang should
unbundle the bundled preprocessor output and launch
device and host compilation actions.

Currently there is a bug in clang driver causing bundled
preprocessor output not unbundled.

This patch fixes that.

Diff Detail

Event Timeline

yaxunl requested review of this revision.Dec 5 2020, 6:35 AM
yaxunl created this revision.
tra added a comment.Dec 7 2020, 10:55 AM

-E by default prints preprocessed output to stdout. CUDA will print preprocessed output from all subcompilations. What does HIP do in this case? Printing out the bundle is probably not what the user will expect.
IMO preprocessed output is frequently used as a debugging tool, so it's important for users to be able to read it. Bundled output is rather cumbersome to deal with. It's possible to manually unbundle it, but the tool is not documented well and it's not particularly suitable for human use.

I think we should preserve the long-established meaning of -E and keep its output as plain text.

Perhaps we need a flag telling the driver to bundle the outputs. Somthing like -fhip-bundle-outputs. I think it may be useful in other cases. E.g. what if we want so compile to assembly and then assemble to the object file. It's not that different from preprocess and compile you're trying to achieve here. This may be a more general solution.

clang/lib/Driver/Driver.cpp
2523–2526

Nit: too many negations in the condition. if (! (IA->getType() == types::TY_CUDA || IA->getType() == types::TY_HIP || IA->getType() == types::TY_PP_HIP)) would be easier to understand, IMO.

yaxunl updated this revision to Diff 311225.Dec 11 2020, 7:18 AM

revised by Artem's comments

yaxunl marked an inline comment as done.Dec 11 2020, 7:24 AM
In D92720#2437621, @tra wrote:

-E by default prints preprocessed output to stdout. CUDA will print preprocessed output from all subcompilations. What does HIP do in this case? Printing out the bundle is probably not what the user will expect.
IMO preprocessed output is frequently used as a debugging tool, so it's important for users to be able to read it. Bundled output is rather cumbersome to deal with. It's possible to manually unbundle it, but the tool is not documented well and it's not particularly suitable for human use.

I think we should preserve the long-established meaning of -E and keep its output as plain text.

Output of -E for HIP combined host/device compilation is a plain text. It has C++ comments inserted between preprocessor outputs for host and different GPU arch's. The C++ comments follow the format of clang-offload-bundler bundled text files therefore clang-offload-bundler is able to unbundle it.

clang/lib/Driver/Driver.cpp
2523–2526

done

tra accepted this revision.Dec 14 2020, 3:03 PM

Output of -E for HIP combined host/device compilation is a plain text. It has C++ comments inserted between preprocessor outputs for host and different GPU arch's. The C++ comments follow the format of clang-offload-bundler bundled text files therefore clang-offload-bundler is able to unbundle it.

OK.

This actually exposed a minor issue. Using something that may legitemately occur in the user source as a separator is rather easy to break.
E.g. there's nothing wrong with the following code, but the bundler will not handle it well if it's used with hip-cpp-output:

const char* s1 = R"foo(
// __CLANG_OFFLOAD_BUNDLE____END__ hip-amdgcn-amd-amdhsa-gfx803
)foo";

It's not a showstopper, but it would be great if the separator would be something that can't be encountered in the preprocessed output.

This revision is now accepted and ready to land.Dec 14 2020, 3:03 PM
yaxunl marked an inline comment as done.Dec 14 2020, 8:26 PM
In D92720#2453277, @tra wrote:

Output of -E for HIP combined host/device compilation is a plain text. It has C++ comments inserted between preprocessor outputs for host and different GPU arch's. The C++ comments follow the format of clang-offload-bundler bundled text files therefore clang-offload-bundler is able to unbundle it.

OK.

This actually exposed a minor issue. Using something that may legitemately occur in the user source as a separator is rather easy to break.
E.g. there's nothing wrong with the following code, but the bundler will not handle it well if it's used with hip-cpp-output:

const char* s1 = R"foo(
// __CLANG_OFFLOAD_BUNDLE____END__ hip-amdgcn-amd-amdhsa-gfx803
)foo";

It's not a showstopper, but it would be great if the separator would be something that can't be encountered in the preprocessed output.

That's a good point. Need to think about a better way even though the chances of encountering such issue are low.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 7:14 PM