[Driver][SYCL] Improve FPGA archive device unbundling with AOCO #9572

maksimsab · 2023-05-23T16:22:50Z

AOCO specific objects can be added to an archive. When this occurs,
the archive was being completely ignored, causing issues with device
compilation. Take advantage of the additional -excluded-targets option
from the clang-offload-bundler so we can process these archives for
the needed device information, excluding potential unbundling of the
target device in the presense of aoco binaries in that same object.

asudarsa · 2023-05-24T13:57:01Z

Hi @maksimsab

Can you please add some description to this PR?

Thanks

clang/lib/Driver/Driver.cpp

clang/lib/Driver/OffloadBundler.cpp

asudarsa

Bundler changes LGTM. Thanks

AOCO specific objects can be added to an archive. When this occurs, the archive was being completely ignored, causing issues with device compilation. Take advantage of the additional -excluded-targets option from the clang-offload-bundler so we can process these archives for the needed device information, excluding potential unbundling of the target device in the presense of aoco binaries in that same object. Co-authored-by: Michael Toguchi <michael.d.toguchi@intel.com>

hchilama

Driver changes LGTM, thanks

asudarsa · 2023-05-26T11:01:11Z

clang/lib/Driver/OffloadBundler.cpp

@@ -1215,6 +1241,66 @@ class ArchiveFileHandler final : public FileHandler {
  Error WriteBundle(raw_fd_ostream &OS, MemoryBuffer &Input) override {
    llvm_unreachable("unsupported for the ArchiveFileHandler");
  }
+
+private:


We are opening and parsing the file here an additional time for every child. This can prove expensive. There must be a way to do this 'exclusion' on the fly using a single parse of the file. But we can do that as a subsequent work. Please add a TODO here.

I thought about this optimizations.
There is a cost we should always consider is that this tool is from community. And we should resolve conflicts in sycl-web every time something comes from community in this functionality. So if we change the archive processing significantly in terms of a code then conflict resolving could turn into hard work for one who will maintain this stuff (it is me or somebody from our team).

Here it is. It removes useless work in general case.

3707aa4#diff-c29010597bfa221cf7e7cfc018e0b0a7bdae17bcf8d17b27be96f4cbd20ec28aR1293-R1295

This is surely not a blocker for this PR. But we need to address this and atleast verify that this does not increase compile time by a lot. Thanks

clang/lib/Driver/OffloadBundler.cpp

asudarsa

LGTM. Few opportunities for optimization can be addressed later. Thanks

AlexeySachkov

Strictly speaking, there are no tests for clang-offload-bundler changes in this PR.

AlexeySachkov · 2023-05-26T13:29:03Z

clang/lib/Driver/OffloadBundler.cpp

+    // NOTE: "-sycldevice" Triple component has been deprecated.
+    // However, it still can be met in libraries that have been compiled before
+    // deprecation. For example, here Triple might be the following:
+    //  sycl-fpga_aoco-intel-unknown-sycldevice
+    //
+    // The workaround is to strip this Triple component if it is present.
+    Triple.consume_back("-sycldevice");


We have dropped support for such binaries, no need to have that workaround anymore, see #9408

Well, it looks like FPGA libraries still depend on it. We asked them how did it happen. But it doesn't look like we can drop support of "sycldevice" env.

We have effectively done that already. We do aim to preserve backwards compatibility generally, but right now we are in ABI-breaking window and we are allowed to cleanup legacy code, thus breaking backwards compatibility.

clang/lib/Driver/OffloadBundler.cpp

AlexeySachkov · 2023-05-26T13:33:57Z

clang/lib/Driver/OffloadBundler.cpp

+    for (const auto &TargetName : TargetNames)
+      if (CheckTargetIsExcluded(TargetName))
+        return true;
+
+    return false;


Return std::count_if() > 0 here as well? cppreference

CheckTargetIsExcluded is a method. Thus, it can't be passed as UnaryPredicate. Also it can't be made static because it uses BundlerConfig. Trying to make it static and pass BundlerConfig through would make the code look ugly.

I think it can be converted into a lamda, since it is fairly small method. But feel free to disregard this comment or apply in a separate follow-up patch

clang/lib/Driver/OffloadBundler.cpp

mdtoguchi · 2023-05-26T17:28:07Z

clang/test/Driver/clang-offload-bundler-exclude.c

+// RUN: llc -filetype=obj -o %t-device2.o %t-device2.bc
+// RUN: llc -filetype=obj -o %t-aoco.o %t-aoco.bc
+// RUN: llc -filetype=obj -o %t-host.o %t-host.bc
+// RUN: llc -filetype=obj -o %t-host2.o %t-host2.bc


Does it make sense to use a pre-built binary/archive and put it in Inputs/SYCL and just test the behavior of clang-offload-bundler? It reduces external dependencies on other tools.

I can put it into Inputs folder. However, I still have dependency on cat, xargs, strings and grep tools in the final line of the test.

aelovikov-intel · 2023-05-30T15:10:57Z

I see

OffloadBundler.cpp:1259:14: error: call to deleted constructor of 'llvm::Error'
      return Err;

locally.

I believe the issue has been introduced in #9572.

maksimsab force-pushed the offload_bundler branch from 68d0cce to 18b7c46 Compare May 24, 2023 13:24

maksimsab temporarily deployed to aws May 24, 2023 13:50 — with GitHub Actions Inactive

asudarsa reviewed May 24, 2023

View reviewed changes

clang/lib/Driver/Driver.cpp Show resolved Hide resolved

asudarsa reviewed May 24, 2023

View reviewed changes

clang/lib/Driver/OffloadBundler.cpp Outdated Show resolved Hide resolved

asudarsa reviewed May 24, 2023

View reviewed changes

clang/lib/Driver/OffloadBundler.cpp Outdated Show resolved Hide resolved

asudarsa reviewed May 24, 2023

View reviewed changes

clang/lib/Driver/OffloadBundler.cpp Outdated Show resolved Hide resolved

asudarsa reviewed May 24, 2023

View reviewed changes

clang/lib/Driver/OffloadBundler.cpp Outdated Show resolved Hide resolved

maksimsab temporarily deployed to aws May 24, 2023 14:21 — with GitHub Actions Inactive

maksimsab force-pushed the offload_bundler branch from 18b7c46 to 7a9bd3b Compare May 24, 2023 14:31

asudarsa approved these changes May 24, 2023

View reviewed changes

maksimsab temporarily deployed to aws May 24, 2023 15:34 — with GitHub Actions Inactive

maksimsab force-pushed the offload_bundler branch from 7a9bd3b to 992ed07 Compare May 24, 2023 15:43

maksimsab force-pushed the offload_bundler branch from 992ed07 to 9aa6c0f Compare May 24, 2023 15:44

maksimsab changed the title ~~[SYCL][NFC] add excluded-targets option in clang-offload-bundler~~ [Driver][SYCL] Improve FPGA archive device unbundling with AOCO May 24, 2023

maksimsab marked this pull request as ready for review May 24, 2023 15:48

maksimsab requested review from a team as code owners May 24, 2023 15:48

maksimsab requested a review from mdtoguchi May 24, 2023 15:50

maksimsab temporarily deployed to aws May 24, 2023 16:15 — with GitHub Actions Inactive

maksimsab temporarily deployed to aws May 24, 2023 16:47 — with GitHub Actions Inactive

MrSidims approved these changes May 25, 2023

View reviewed changes

[SYCL] reimplement -exclude-targets option

c3e9c7f

maksimsab temporarily deployed to aws May 25, 2023 17:02 — with GitHub Actions Inactive

maksimsab temporarily deployed to aws May 25, 2023 17:34 — with GitHub Actions Inactive

hchilama approved these changes May 25, 2023

View reviewed changes

asudarsa reviewed May 26, 2023

View reviewed changes

clang/lib/Driver/OffloadBundler.cpp Outdated Show resolved Hide resolved

[SYCL] optimize CheckObjectFileContainsExcludedTargets for general case

3707aa4

[SYCL] address code review

e12cf63

asudarsa approved these changes May 26, 2023

View reviewed changes

maksimsab temporarily deployed to aws May 26, 2023 11:46 — with GitHub Actions Inactive

maksimsab temporarily deployed to aws May 26, 2023 12:17 — with GitHub Actions Inactive

AlexeySachkov reviewed May 26, 2023

View reviewed changes

[SYCL] address code review

65793fe

maksimsab temporarily deployed to aws May 26, 2023 15:03 — with GitHub Actions Inactive

maksimsab temporarily deployed to aws May 26, 2023 15:34 — with GitHub Actions Inactive

[SYCL] add test for -exclude-target functionality

3160dad

maksimsab temporarily deployed to aws May 26, 2023 16:11 — with GitHub Actions Inactive

[SYCL] turn off test for Windows

eae316e

mdtoguchi reviewed May 26, 2023

View reviewed changes

maksimsab temporarily deployed to aws May 26, 2023 17:53 — with GitHub Actions Inactive

maksimsab temporarily deployed to aws May 26, 2023 18:24 — with GitHub Actions Inactive

maksimsab added 2 commits May 30, 2023 04:39

[SYCL] add fatlib.a in Inputs folder

35b6c0c

[SYCL] fix naming

fa54c9e

maksimsab temporarily deployed to aws May 30, 2023 12:06 — with GitHub Actions Inactive

maksimsab temporarily deployed to aws May 30, 2023 12:38 — with GitHub Actions Inactive

mdtoguchi approved these changes May 30, 2023

View reviewed changes

AlexeySachkov merged commit 4ed3676 into intel:sycl May 30, 2023

aelovikov-intel mentioned this pull request May 30, 2023

[OffloadBundler] Fix build with clang-10 #9649

Merged

maksimsab deleted the offload_bundler branch May 30, 2023 17:07

aelovikov-intel added a commit that referenced this pull request May 30, 2023

[OffloadBundler] Fix build with clang-10 (#9649)

8cceeb5

I believe the issue has been introduced in #9572.

[Driver][SYCL] Improve FPGA archive device unbundling with AOCO #9572

[Driver][SYCL] Improve FPGA archive device unbundling with AOCO #9572

Uh oh!

Conversation

maksimsab commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asudarsa commented May 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

hchilama left a comment

Choose a reason for hiding this comment

Uh oh!

asudarsa May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maksimsab May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented May 30, 2023

Uh oh!

Uh oh!

maksimsab commented May 23, 2023 •

edited

Loading

asudarsa May 26, 2023 •

edited

Loading

maksimsab May 26, 2023 •

edited

Loading