[sycl-post-link] Refactor cloned modules processing #4828

mlychkov · 2021-10-27T10:03:18Z

ValueToValueMapTy map is not fully release on destruction the data that is
allocated on CloneModule call. Also split modules may utilize tens of megabytes
and their amount may be significant. It all may lead to overflow of RAM memory.

Rewrite module splitting in a way that each split module is processed and
immediately saved into a file without temporary collecting all modules in RAM.
Also try to share one ValueToValueMapTy instance for all cloned modules. It
may reduce memory utilization, but in some cases it also may loose sensitive data.
Add an option "-reduce-memory-usage" that turns on value map sharing mode.
This mode is off by default.

Signed-off-by: Mikhail Lychkov [email protected]

mlychkov · 2021-10-27T10:25:54Z

Temporary moved to draft mode to check with users that new solution doesn't break result applications functionality.

ValueToValueMapTy map is not fully release on destruction the data that is allocated on CloneModule call. Also split modules may utilize tens of megabytes and their amount may be significant. It all may lead to overflow of RAM memory. Rewrite module splitting in a way that each split module is processed and immediately saved into a file without temporary collecting all modules in RAM. Signed-off-by: Mikhail Lychkov <[email protected]>

Signed-off-by: Mikhail Lychkov <[email protected]>

mlychkov · 2021-10-28T14:30:15Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+      std::string ResModuleFile{};
+      bool CanReuseInputModule = !SyclAndEsimdCode && !IsEsimd &&
+                                 !IsLLVMUsedRemoved && !SpecConstsMet &&
+                                 (!DoSplit || GlobalsSet.size() <= 1);


The last condition is equal to ResultModules.size() == 1 in previous implementation.
I'd like just to clarify if this is correct to reuse input module when split is requested and GlobalsSet.size() == 1. This means that split scope is global, or split scope is per_source and all entry points are in 1 source file, or split is per_kernel and there is only 1 entry point. Anyway in such a situation CloneModule is called and some code optimizations are performed during splitting.
May we reuse input module in this case or this case should be excluded from check?

May we reuse input module in this case or this case should be excluded from check?

I think it is ok to reuse input module in this case, because absolutely the same situation as one of you described here can happen with the current implementation as well - and we will reuse input module in that case.

Then there are 2 concerns:

If we can reuse input module when splitting is used and GlobalsSet.size() == 1 then splitting is useless because the result is not saved. We might save resources and just use input module instead of split one.

The results when we can reuse input module and when for example llvm.used or spec const is added into the same input module will differ a lot because in the first case we will split module and just drop the result reusing input module, but in second case we will split module, optimize it and save into a file.

llvm/tools/sycl-post-link/sycl-post-link.cpp

AlexeySachkov · 2021-10-28T18:15:50Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+  // This map is shared between calls to fix memory leaks after its deletion and
+  // reduce number of allocations and deallocations of memory for Module objects
+  // during llvm::CloneModule call.
+  static ValueToValueMapTy VMap;
+
+  // During cloning of Module ValueMap tries to reuse already created old to new
+  // Value mappings and asserts if a new Value pointer is nullptr. It may happen
+  // after one cloned module is destroyed and we try to reuse ValueToValueMapTy
+  // object for the next module to clone.
+  // Proper solution is to improve CloneModule to support multiple clonings from
+  // one source Module.
+  // Current workaround is to drop all records that contain nullptr values.
+  for (auto It = VMap.begin(), End = VMap.end(); It != End;) {
+    if (It->second)
+      ++It;
+    else
+      VMap.erase(It++);
+  }


This still looks like a hack/workaround to me. I don't want to block the commit with this, but it would be great if we find root cause of ValueToValueMapTy not being deallocated properly to check if there is a more elegant solution to the problem

Signed-off-by: Mikhail Lychkov <[email protected]>

mlychkov · 2021-11-01T13:16:31Z

/summary:run

Signed-off-by: Mikhail Lychkov <[email protected]>

…efactoring

Signed-off-by: Mikhail Lychkov <[email protected]>

mlychkov · 2021-11-03T07:33:53Z

/summary:run

…efactoring

mlychkov · 2021-11-16T13:30:55Z

/summary:run

mlychkov · 2021-11-17T05:56:36Z

Looks like test failure is a flaky known issue with llvm_test_suite_sycl/reduction_placeholder test abort after pass on L0 GPU target.

kbobrovs · 2021-11-18T23:49:17Z

Also try to share one ValueToValueMapTy instance for all cloned modules. It
may reduce memory utilization, but in some cases it also may loose sensitive data.
Add an option "-reduce-memory-usage" that turns on value map sharing mode.
This mode is off by default.

Does loosing sensitive data lead to incorrect results/behavior in some cases? If so I don't think we should add this option.
Is it possible to automatically see if it is safe and apply the memory reduction only in that case (w/o option)?

kbobrovs · 2021-11-18T23:57:27Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

 using TableFiles = std::map<StringRef, string_vector>;

-TableFiles processOneModule(std::unique_ptr<Module> M, bool IsEsimd,
-                            bool SyclAndEsimdCode) {
+struct SplitModeInfo {


It is quite hard to reverse engineer the overall algorithm/design. Can it be discussed online?

How much memory saving ReduceMemUsage brings?

mlychkov · 2021-11-24T13:57:23Z

The patch is reorganized in a set of newer PRs to simplify review process.

mlychkov requested review from bader, AlexeySachkov and kbobrovs October 27, 2021 10:03

mlychkov marked this pull request as draft October 27, 2021 10:24

mlychkov force-pushed the private/mlychkov/sycl_post_link_1mod_refactoring branch from 012fa96 to 05a4b07 Compare October 27, 2021 10:37

mlychkov force-pushed the private/mlychkov/sycl_post_link_1mod_refactoring branch from 05a4b07 to 5d80d3b Compare October 27, 2021 10:37

mlychkov added 2 commits October 28, 2021 12:20

Fix linux LIT test

7a8d9b8

Signed-off-by: Mikhail Lychkov <[email protected]>

Changed indexing in module processing loop

838947e

Signed-off-by: Mikhail Lychkov <[email protected]>

mlychkov requested a review from sndmitriev October 28, 2021 11:10

Fix linux LIT tests after previous improvement

5534766

mlychkov commented Oct 28, 2021

View reviewed changes

AlexeySachkov reviewed Oct 28, 2021

View reviewed changes

mlychkov added 5 commits October 29, 2021 16:42

Fix crash when a module with sycl and esimd modules is split

e3030b9

Fix sort.cpp test crash

9138adb

Signed-off-by: Mikhail Lychkov <[email protected]>

Cleanup code

430abad

Signed-off-by: Mikhail Lychkov <[email protected]>

Fix clang format

ebfe813

Clarify comment

3ecb57d

mlychkov added 4 commits November 2, 2021 13:53

Add option turning on shared value map mode

fd9c973

Signed-off-by: Mikhail Lychkov <[email protected]>

Merge remote-tracking branch 'origin/sycl' into sycl_post_link_1mod_r…

12d4488

…efactoring

Fix clang format and add regression test for CUDA

30849f6

Signed-off-by: Mikhail Lychkov <[email protected]>

Fix LIT tests

e865ec1

mlychkov added 5 commits November 4, 2021 12:05

Update sycl-post-link tests

7bdaec8

Merge remote-tracking branch 'origin/sycl' into sycl_post_link_1mod_r…

9e04aee

…efactoring

Remove unused variables

5edf930

Add empty globals vector if globals set is empty

609fb7b

Fix formatting

d1f612e

mlychkov added 5 commits November 11, 2021 11:26

Fix LIT test

97fc261

More robust but time-consuming splitting mechanism

295a791

Fix new splitting algo

eca6b7a

Fix getting module functions

da87237

Fix formatting

c849fe7

Minor updates

3c5dc6b

mlychkov mentioned this pull request Nov 18, 2021

[sycl-post-link][NFC] Small cleanups and cosmetic changes #4986

Merged

kbobrovs reviewed Nov 19, 2021

View reviewed changes

mlychkov closed this Nov 24, 2021

mlychkov deleted the private/mlychkov/sycl_post_link_1mod_refactoring branch November 24, 2021 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[sycl-post-link] Refactor cloned modules processing #4828

[sycl-post-link] Refactor cloned modules processing #4828

Uh oh!

mlychkov commented Oct 27, 2021 •

edited

Loading

Uh oh!

mlychkov commented Oct 27, 2021

Uh oh!

mlychkov Oct 28, 2021

Uh oh!

AlexeySachkov Oct 28, 2021

Uh oh!

mlychkov Oct 29, 2021

Uh oh!

Uh oh!

AlexeySachkov Oct 28, 2021

Uh oh!

mlychkov commented Nov 1, 2021

Uh oh!

mlychkov commented Nov 3, 2021

Uh oh!

mlychkov commented Nov 16, 2021

Uh oh!

mlychkov commented Nov 17, 2021 •

edited

Loading

Uh oh!

kbobrovs commented Nov 18, 2021

Uh oh!

kbobrovs Nov 18, 2021

Uh oh!

mlychkov commented Nov 24, 2021

Uh oh!

Uh oh!

[sycl-post-link] Refactor cloned modules processing #4828

[sycl-post-link] Refactor cloned modules processing #4828

Uh oh!

Conversation

mlychkov commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlychkov commented Oct 27, 2021

Uh oh!

mlychkov Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

mlychkov Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

mlychkov commented Nov 1, 2021

Uh oh!

mlychkov commented Nov 3, 2021

Uh oh!

mlychkov commented Nov 16, 2021

Uh oh!

mlychkov commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbobrovs commented Nov 18, 2021

Uh oh!

kbobrovs Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

mlychkov commented Nov 24, 2021

Uh oh!

Uh oh!

mlychkov commented Oct 27, 2021 •

edited

Loading

mlychkov commented Nov 17, 2021 •

edited

Loading