[HIP] unbundle bundled preprocessor output

[yaxunl]

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.

[tra]

-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

2463–2466

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]

revised by Artem's comments

[yaxunl]

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

2463–2466

done

[tra]

In D92720#2448890, @yaxunl 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.

[yaxunl]

In D92720#2453277, @tra wrote:

In D92720#2448890, @yaxunl 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.

clang/lib/Driver/Driver.cpp

ActionBuilderReturnCode addDeviceDepences(Action *HostAction) override {
       // architecture. If we are in host-only mode we return 'success' so that
       // the host uses the CUDA offload kind.
       if (auto *IA = dyn_cast<InputAction>(HostAction)) {
         assert(!GpuArchList.empty() &&
                "We should have at least one GPU architecture.");
 
         // If the host input is not CUDA or HIP, we don't need to bother about
         // this input.
-        if (IA->getType() != types::TY_CUDA &&
-            IA->getType() != types::TY_HIP) {
+        if (!(IA->getType() == types::TY_CUDA ||
+              IA->getType() == types::TY_HIP ||
+              IA->getType() == types::TY_PP_HIP)) {
           // The builder will ignore this input.
           IsActive = false;
           return ABRT_Inactive;
         }
 
         // Set the flag to true, so that the builder acts on the current input.
         IsActive = true;
 
         if (CompileHostOnly)
ActionBuilderReturnCode addDeviceDepences(Action *HostAction) override {
         return ABRT_Success;
       }
 
       // If this is an unbundling action use it as is for each CUDA toolchain.
       if (auto *UA = dyn_cast<OffloadUnbundlingJobAction>(HostAction)) {
 
         // If -fgpu-rdc is disabled, should not unbundle since there is no
         // device code to link.
-        if (!Relocatable)
+        if (UA->getType() == types::TY_Object && !Relocatable)
           return ABRT_Inactive;
 
         CudaDeviceActions.clear();
         auto *IA = cast<InputAction>(UA->getInputs().back());
         std::string FileName = IA->getInputArg().getAsString(Args);
         // Check if the type of the file is the same as the action. Do not
         // unbundle it if it is not. Do not unbundle .so files, for example,
         // which are not object files.
bool addHostDependenceToDeviceActions(Action *&HostAction,
     // If we are supporting bundling/unbundling and the current action is an
     // input action of non-source file, we replace the host action by the
     // unbundling action. The bundler tool has the logic to detect if an input
     // is a bundle or not and if the input is not a bundle it assumes it is a
     // host file. Therefore it is safe to create an unbundling action even if
     // the input is not a bundle.
     if (CanUseBundler && isa<InputAction>(HostAction) &&
         InputArg->getOption().getKind() == llvm::opt::Option::InputClass &&
-        !types::isSrcFile(HostAction->getType())) {
+        (!types::isSrcFile(HostAction->getType()) ||
+         HostAction->getType() == types::TY_PP_HIP)) {
       auto UnbundlingHostAction =
           C.MakeAction<OffloadUnbundlingJobAction>(HostAction);
       UnbundlingHostAction->registerDependentActionInfo(
           C.getSingleOffloadToolChain<Action::OFK_Host>(),
           /*BoundArch=*/StringRef(), Action::OFK_Host);
       HostAction = UnbundlingHostAction;
     }
 

clang/test/Driver/hip-unbundle-preproc.hip

+// REQUIRES: clang-driver, amdgpu-registered-target
+
+// RUN: %clang -### -target x86_64-unknown-linux-gnu \
+// RUN:   --offload-arch=gfx803 -nogpulib \
+// RUN:   -x hip-cpp-output %s 2>&1 | FileCheck %s
+
+// CHECK: {{".*clang-offload-bundler.*"}} {{.*}}"-outputs=[[HOST_PP:.*cui]],[[DEV_PP:.*cui]]" "-unbundle"
+// CHECK: {{".*clang.*"}} "-cc1" {{.*}}"-target-cpu" "gfx803" {{.*}}"-o" "[[DEV_O:.*o]]" {{.*}}"[[DEV_PP]]"
+// CHECK: {{".*lld.*"}} {{.*}}"-o" "[[DEV_ISA:.*]]" "[[DEV_O]]"
+// CHECK: {{".*clang-offload-bundler.*"}} {{.*}}"-inputs={{.*}},[[DEV_ISA]]" "-outputs=[[FATBIN:.*]]"
+// CHECK: {{".*clang.*"}} {{.*}}"-triple" "x86_64-unknown-linux-gnu"{{.*}} "-fcuda-include-gpubinary" "[[FATBIN]]" {{.*}}"-o" "[[HOST_O:.*o]]" {{.*}}"[[HOST_PP]]"
+// CHECK: {{".*ld.*"}} {{.*}}"[[HOST_O]]"
+
+// RUN: %clang -### -target x86_64-unknown-linux-gnu \
+// RUN:   --offload-arch=gfx803 -nogpulib -fgpu-rdc \
+// RUN:   -x hip-cpp-output %s 2>&1 | FileCheck -check-prefix=RDC %s
+
+// RDC: {{".*clang-offload-bundler.*"}} {{.*}}"-outputs=[[HOST_PP:.*cui]],[[DEV_PP:.*cui]]" "-unbundle"
+// RDC: {{".*clang.*"}} {{.*}}"-triple" "x86_64-unknown-linux-gnu"{{.*}} "-o" "[[HOST_O:.*o]]" {{.*}}"[[HOST_PP]]"
+// RDC: {{".*clang-offload-bundler.*"}} {{.*}}"-outputs=[[HOST_PP:.*cui]],[[DEV_PP:.*cui]]" "-unbundle"
+// RDC: {{".*clang.*"}} "-cc1" {{.*}}"-target-cpu" "gfx803" {{.*}}"-o" "[[DEV_BC:.*bc]]" {{.*}}"[[DEV_PP]]"
+// RDC: {{".*lld.*"}} {{.*}}"-o" "[[DEV_ISA:.*]]" "[[DEV_BC]]"
+// RDC: {{".*clang-offload-bundler.*"}} {{.*}}"-inputs={{.*}},[[DEV_ISA]]" "-outputs=[[FATBIN:.*]]"
+// RDC: {{".*llvm-mc.*"}} "-o" "[[FATBIN_O:.*o]]"
+// RDC: {{".*ld.*"}} {{.*}}"[[HOST_O]]" "[[FATBIN_O]]"