Page MenuHomePhabricator

[Driver][OpenMP] Add support to create jobs for unbundling actions.
ClosedPublic

Authored by sfantao on Jun 29 2016, 12:08 PM.

Details

Summary

This patch adds the support to create jobs for the OffloadBundlingAction which will invoke the clang-offload-bundler tool to unbundle input files.

Unlike other actions, unbundling actions have multiple outputs. Therefore, this patch adds the required changes to have a variant of Tool::ConstructJob with multiple outputs.

The way the naming of the results is implemented is also slightly modified so that the same action can use a different offloading prefix for each use by the different offloading actions.

With this patch, it is possible to compile a functional OpenMP binary with offloading support, even with separate compilation.

Event Timeline

sfantao updated this revision to Diff 62264.Jun 29 2016, 12:08 PM
sfantao retitled this revision from to [Driver][OpenMP] Add support to create jobs for unbundling actions..
sfantao updated this object.
sfantao added reviewers: echristo, tra, jlebar, hfinkel, ABataev.
sfantao updated this object.Jun 29 2016, 12:12 PM
ABataev edited edge metadata.Jun 29 2016, 8:18 PM

Remove '\brief'

include/clang/Driver/Action.h
160–162

Can this be 'static'? Also, name of the function must start from capital letter.

include/clang/Driver/Tool.h
132–140

Please, format properly. Remove the name of the function and '-' signs after params

sfantao updated this revision to Diff 62569.Jul 1 2016, 4:34 PM
sfantao marked 2 inline comments as done.
sfantao edited edge metadata.
  • Change name of static function, format comments and remove \brief.

Hi Alexey, thanks for the review!

include/clang/Driver/Tool.h
132–140

Ok, I was following the format used for the other version of the is function.

sfantao updated this revision to Diff 62591.Jul 1 2016, 5:37 PM
  • Fix format.

Hi Samuel,

this looks pretty good overall!

I've only encountered one issue which I don't really know is caused by which patch: I can't use the (exact) same triple for offloading and the host when compiling separately. I found out that the device object file is taken as input for the final link step and ld therefore complains

<...>/lib64/crt1.o: In function `_start':
(.text+0x20): undefined reference to `main'

I've not yet found the reason - are there some unordered_maps that don't keep the order? I'm also seeing that the first argument to clang-offload-bundler is not always for the host...

Thanks,
Jonas

Hi Samuel,

this looks pretty good overall!

I've only encountered one issue which I don't really know is caused by which patch: I can't use the (exact) same triple for offloading and the host when compiling separately. I found out that the device object file is taken as input for the final link step and ld therefore complains

<...>/lib64/crt1.o: In function `_start':
(.text+0x20): undefined reference to `main'

I've not yet found the reason - are there some unordered_maps that don't keep the order? I'm also seeing that the first argument to clang-offload-bundler is not always for the host...

Thanks,
Jonas

Thanks for letting me know! I'll investigate and see if I can replicate the issue here.

Thanks again!
Samuel

Hahnfeld added inline comments.Jul 8 2016, 6:48 AM
lib/Driver/Driver.cpp
3155–3162

I think A->getOffloadingDeviceKind() will always be OFK_None here because A is of type OffloadUnbundlingJobAction. What we need here instead is the OffloadKind that is building the job for the unbundling action.

The following patch fixes separate compilation with the same triple for me (including the needed change mentioned in D21847#inline-187652)

diff --git a/include/clang/Driver/Driver.h b/include/clang/Driver/Driver.h
index 7abd9eb..72d19b2 100644
--- a/include/clang/Driver/Driver.h
+++ b/include/clang/Driver/Driver.h
@@ -12,6 +12,7 @@
 
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LLVM.h"
+#include "clang/Driver/Action.h"
 #include "clang/Driver/Phases.h"
 #include "clang/Driver/Types.h"
 #include "clang/Driver/Util.h"
@@ -44,7 +45,6 @@ class FileSystem;
 
 namespace driver {
 
-  class Action;
   class Command;
   class Compilation;
   class InputInfo;
@@ -423,7 +423,7 @@ public:
                      const char *LinkingOutput,
                      std::map<std::pair<const Action *, std::string>, InputInfo>
                          &CachedResults,
-                     bool BuildForOffloadDevice) const;
+                     Action::OffloadKind BuildForOffloadKind) const;
 
   /// Returns the default name for linked images (e.g., "a.out").
   const char *getDefaultImageName() const;
@@ -491,7 +491,7 @@ private:
       const char *LinkingOutput,
       std::map<std::pair<const Action *, std::string>, InputInfo>
           &CachedResults,
-      bool BuildForOffloadDevice) const;
+      Action::OffloadKind BuildForOffloadKind) const;
 
 public:
   /// GetReleaseVersion - Parse (([0-9]+)(.([0-9]+)(.([0-9]+)?))?)? and
diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
index 6221a05..4d451f4 100644
--- a/lib/Driver/Driver.cpp
+++ b/lib/Driver/Driver.cpp
@@ -2554,7 +2554,7 @@ void Driver::BuildJobs(Compilation &C) const {
                        /*AtTopLevel*/ true,
                        /*MultipleArchs*/ ArchNames.size() > 1,
                        /*LinkingOutput*/ LinkingOutput, CachedResults,
-                       /*BuildForOffloadDevice*/ false);
+                       Action::OFK_None);
   }
 
   // If the user passed -Qunused-arguments or there were errors, don't warn
@@ -2904,9 +2904,9 @@ static std::string GetTriplePlusArchString(const ToolChain *TC,
   if (BoundArch) {
     TriplePlusArch += "-";
     TriplePlusArch += BoundArch;
-    TriplePlusArch += "-";
-    TriplePlusArch += Action::GetOffloadKindName(OffloadKind);
   }
+  TriplePlusArch += "-";
+  TriplePlusArch += Action::GetOffloadKindName(OffloadKind);
   return TriplePlusArch;
 }
 
@@ -2914,16 +2914,16 @@ InputInfo Driver::BuildJobsForAction(
     Compilation &C, const Action *A, const ToolChain *TC, const char *BoundArch,
     bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput,
     std::map<std::pair<const Action *, std::string>, InputInfo> &CachedResults,
-    bool BuildForOffloadDevice) const {
+    Action::OffloadKind BuildForOffloadKind) const {
   std::pair<const Action *, std::string> ActionTC = {
-      A, GetTriplePlusArchString(TC, BoundArch, A->getOffloadingDeviceKind())};
+      A, GetTriplePlusArchString(TC, BoundArch, BuildForOffloadKind)};
   auto CachedResult = CachedResults.find(ActionTC);
   if (CachedResult != CachedResults.end()) {
     return CachedResult->second;
   }
   InputInfo Result = BuildJobsForActionNoCache(
       C, A, TC, BoundArch, AtTopLevel, MultipleArchs, LinkingOutput,
-      CachedResults, BuildForOffloadDevice);
+      CachedResults, BuildForOffloadKind);
   CachedResults[ActionTC] = Result;
   return Result;
 }
@@ -2932,9 +2932,12 @@ InputInfo Driver::BuildJobsForActionNoCache(
     Compilation &C, const Action *A, const ToolChain *TC, const char *BoundArch,
     bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput,
     std::map<std::pair<const Action *, std::string>, InputInfo> &CachedResults,
-    bool BuildForOffloadDevice) const {
+    Action::OffloadKind BuildForOffloadKind) const {
   llvm::PrettyStackTraceString CrashInfo("Building compilation jobs");
 
+  bool BuildForOffloadDevice =
+      BuildForOffloadKind != Action::OFK_None &&
+          BuildForOffloadKind != Action::OFK_None;
   InputInfoList OffloadDependencesInputInfo;
   if (const OffloadAction *OA = dyn_cast<OffloadAction>(A)) {
     // The offload action is expected to be used in four different situations.
@@ -2968,7 +2971,7 @@ InputInfo Driver::BuildJobsForActionNoCache(
         DevA =
             BuildJobsForAction(C, DepA, DepTC, DepBoundArch, AtTopLevel,
                                /*MultipleArchs*/ !!DepBoundArch, LinkingOutput,
-                               CachedResults, /*BuildForOffloadDevice=*/true);
+                               CachedResults, DepA->getOffloadingDeviceKind());
       });
       return DevA;
     }
@@ -2983,8 +2986,7 @@ InputInfo Driver::BuildJobsForActionNoCache(
           OffloadDependencesInputInfo.push_back(BuildJobsForAction(
               C, DepA, DepTC, DepBoundArch, /*AtTopLevel=*/false,
               /*MultipleArchs*/ !!DepBoundArch, LinkingOutput, CachedResults,
-              /*BuildForOffloadDevice=*/DepA->getOffloadingDeviceKind() !=
-                  Action::OFK_None));
+              DepA->getOffloadingDeviceKind()));
         });
 
     A = BuildForOffloadDevice
@@ -3017,7 +3019,7 @@ InputInfo Driver::BuildJobsForActionNoCache(
 
     return BuildJobsForAction(C, *BAA->input_begin(), TC, ArchName, AtTopLevel,
                               MultipleArchs, LinkingOutput, CachedResults,
-                              BuildForOffloadDevice);
+                              BuildForOffloadKind);
   }
 
 
@@ -3041,8 +3043,7 @@ InputInfo Driver::BuildJobsForActionNoCache(
           OffloadDependencesInputInfo.push_back(BuildJobsForAction(
               C, DepA, DepTC, DepBoundArch, AtTopLevel,
               /*MultipleArchs=*/!!DepBoundArch, LinkingOutput, CachedResults,
-              /*BuildForOffloadDevice=*/DepA->getOffloadingDeviceKind() !=
-                  Action::OFK_None));
+              DepA->getOffloadingDeviceKind()));
         });
 
   // Only use pipes when there is exactly one input.
@@ -3055,7 +3056,7 @@ InputInfo Driver::BuildJobsForActionNoCache(
         AtTopLevel && (isa<DsymutilJobAction>(A) || isa<VerifyJobAction>(A));
     InputInfos.push_back(BuildJobsForAction(
         C, Input, TC, BoundArch, SubJobAtTopLevel, MultipleArchs, LinkingOutput,
-        CachedResults, BuildForOffloadDevice));
+        CachedResults, A->getOffloadingDeviceKind()));
   }
 
   // Always use the first input as the base input.
@@ -3109,7 +3110,7 @@ InputInfo Driver::BuildJobsForActionNoCache(
     // return for the current depending action.
     std::pair<const Action *, std::string> ActionTC = {
         A,
-        GetTriplePlusArchString(TC, BoundArch, A->getOffloadingDeviceKind())};
+        GetTriplePlusArchString(TC, BoundArch, BuildForOffloadKind)};
     assert(CachedResults.find(ActionTC) != CachedResults.end() &&
            "Result does not exist??");
     Result = CachedResults[ActionTC];
sfantao added inline comments.Jul 8 2016, 6:57 PM
lib/Driver/Driver.cpp
3155–3162

Ok, I see, this is working for me because the host is the first to emit the unbundling option. I'll post a fix along the lines of your patch.

Thanks for catching that!

mkuron added a subscriber: mkuron.Jul 26 2016, 5:27 AM
sfantao updated this revision to Diff 66029.Jul 28 2016, 2:53 PM
  • Fix type qualifier.
  • Rebase.
sfantao updated this revision to Diff 72126.Sep 21 2016, 3:47 PM
  • Rebase.
hfinkel accepted this revision.Sep 28 2016, 12:55 PM
hfinkel edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 28 2016, 12:55 PM
sfantao updated this revision to Diff 76069.Oct 27 2016, 11:23 AM
sfantao edited edge metadata.
  • Rebase.
sfantao closed this revision.Oct 27 2016, 11:24 AM