Fixing for Discarded GOs which are emitted during the second build. #109

MaggieYingYi · 2020-07-21T16:04:56Z

The discardable functions/variables are only pruned if they are referenced by the pruned GOs.

Fix for #55

…GOs. Fix for <#102>

llvm/lib/Transforms/IPO/RepoPruning.cpp

paulhuggett

I really don't fully understand what this proposed change does and why. Please help me understand!

llvm/lib/Transforms/IPO/RepoPruning.cpp

paulhuggett · 2020-08-03T10:19:42Z

llvm/lib/Transforms/IPO/RepoPruning.cpp

+  }
+
+  // Reurn true if the GO is in the new module fragments.
+  auto It = ModuleFragments.find(Digest);


Is ModuleFragments the collection of fragments that will appear in this compilation? Does it include the pruned entries?

Is ModuleFragments the collection of fragments that will appear in this compilation?

Yes, you are right.

Does it include the pruned entries?

No, it doesn't.

paulhuggett · 2020-08-03T15:28:28Z

llvm/lib/Transforms/IPO/RepoPruning.cpp

        // If function 'A' is dependent on function 'B' and 'B' is dependent on
        // function 'C', both RepoDefinition of 'B' and 'C' need to be added
        // into in the 'repo.definitions' during the pruning.
        addDependentFragments(M, DependentFragments, Fragments, Repository,
-                              CM->digest);
+                              XFixupNames, CM->digest);


The comment immediately above this line of code is interesting. Why is this the case?

The comment tries to explain why we need recursive calling addDependentFragments function. It gives an example to demo the case. By the way, it is not added by this PR. The test mentioned in the comment has been added into LLVM test when I fixed the dependent error.

The comment tries to explain why we need recursive calling addDependentFragments function. It gives an example to demo the case. By the way, it is not added by this PR.

Indeed but it does appear to very relevant indeed to the proposed change. In my plea above for help to understand what this does, this seemed particularly pertinent. For the definition of “dependent” in this code you appear to rely on the external fixups rather than the dependents section that I might expect.

Sorry to misunderstand your question. Before discussing the dependent, I want to clarify the following things:

Current Status: The discardable functions/variables are always pruned if they are present in the repository.

The proposed change: The discardable functions/variables are only pruned if they are present in the repository and referenced by other GOs which are in the repository.

To check whether it is referenced by other pruned GOs. I use the external fixups.

Now, back to your question why I need to change addDependentFragments function to collect the external fixups of the fragments in the dependent section?

I want to use an example to explain the reason.

The initial build:

Assuming,

there is a global function A and an internal function B during the RepoMetadataGeneration pass.

a new function is created by other optimisation pass ( which is after the RepoMetadataGeneration pass) and added into the A’s dependent section.

The output compilation contains three members: A, B and D. B is only referenced by D (not by A).

After the initial build, the A, B and D are saved in the repository. A does not have any external fixups.

Build the same file again.

During the pruning stage, there are only A and B (D does not exist).

A is pruned and its external fixups are empty.

D needs to be added into the RepoDefinition metadata and its external fixup needs to be collected. This is the change I added in the function of addDependentFragments: collect the external fixups of fragments in the dependent section.

B is a discardable GO but it is referenced by D, therefore, it can be pruned.

Hope it makes sense. If not, maybe we can arrange a Team Meeting to discuss this in details. 😊

MaggieYingYi · 2020-08-04T14:49:53Z

I really don't fully understand what this proposed change does and why. Please help me understand!

There are two commits in this pull request.

commit: 2b2f0d4.
This commit is a code refactoring for the repo pruning pass. I hope the code more readable after this commit and make the fix for issue55 easier. In this commit, we consider the pruning in two kinds of pruning: the repository level and the module level.
Commit: 2b2f0d4.
This commit is to fix the issue Discarded GOs are emitted during the second build. #55. Do not prune the discardable GO if it is not referenced by the repository level pruned GOs.

As mentioned before, two kinds of pruning are in the RepoPruning pass:

Type 1 is the repository level pruning:
If the global object is compiled before and exists in the repository, it will be pruned.
Type 2 is the module level pruning.
Assuming there is no existing repository. However, in the same compilation unit (module), some global objects have the same digests. The RepoPruning pass could prune them as well.

Consider the following code:

@Var1 = global i32 42
@Var2 = internal global i32 42
@Var3 = weak global i32 42

The Var1, Var2 and Var3 have the same digests but with the different linkage type. After the RepoPruning pass, the IR code changes to:

@Var1 = global i32 42, !repo_definition !0
@Var2 = external global i32, !repo_definition !1
@Var3 = external global i32, !repo_definition !2

We can see that 1) the Var2 and Var3 have been pruned; 2) the RepoPruning could happen even there is no existing database.

The issue #55 is mainly caused by the repository level pruning. If GO is a discarded GO and isn't referenced by the pruned repository level pruned GO, it would be pruned.

Hope this helpful. Please let me know if you need more information.

paulhuggett · 2020-08-05T09:35:00Z

commit 2b2f0d4. This commit is a code refactoring for the repo pruning pass. I hope the code more readable after this commit and make the fix for issue55 easier. In this commit, we consider the pruning in two kinds of pruning: the repository level and the module level.

Commit 2b2f0d4. This commit is to fix the issue Discarded GOs are emitted during the second build. #55. Do not prune the discardable GO if it is not referenced by the repository level pruned GOs.

The first of these commits sounds like much more than a code refactoring if it introduces new concepts! Perhaps we need to discuss this change in a PR of its own…

Type 1 is the repository level pruning:
If the global object is compiled before and exists in the repository, it will be pruned.

This is what we’ve previously considered the role of the pruning pass to be.

Type 2 is the module level pruning.
Assuming there is no existing repository. However, in the same compilation unit (module), some global objects have the same digests. The RepoPruning pass could prune them as well.

Is this a newly introduced feature? What is the motivation behind it? Does this only happen if there is no existing repository (or simply when objects in the compilation are not found in the repo)?

MaggieYingYi · 2020-08-05T13:04:54Z

The first of these commits sounds like much more than a code refactoring if it introduces new concepts! Perhaps we need to discuss this change in a PR of its own…

It didn't introduce any new concepts. These two kinds of pruning always exist in the current master branch. However, they are handled in one single function. The first commit is just a code factoring to make two kinds of pruning more obvious and noticeable.

Is this a newly introduced feature?

No, it exists in the current master branch.

What is the motivation behind it?

I think the motivation is the same as the repository level pruning.

Does this only happen if there is no existing repository (or simply when objects in the compilation are not found in the repo)?

I am not sure I fully understand your question. It does not matter whether there is an existing repository or not. However, the repository level pruning has a high priority. Firstly, the pruning pass will try to find whether there is existing fragment in the repository. If yes, pruning it. Otherwise, the pruning pass will try to find whether there is an existing object in the module.

paulhuggett · 2020-08-05T13:55:46Z

The first of these commits sounds like much more than a code refactoring if it introduces new concepts! Perhaps we need to discuss this change in a PR of its own…

It didn't introduce any new concepts. These two kinds of pruning always exist in the current master branch.

Hmm, okay. These are, however, terms that are new to me. Have I simply forgotten from them from earlier discussions (which would be distressing)? Nonetheless, I think that it would be wise to separate these two changes so that the code relating to the functional change can be easily seen.

I‘ve spent many hours now trying to work out exactly what this change does and why… We seem to be recursively chasing external fixup names and turning them into records in the dependents section. However, I’m not yet completely convinced that this will be correct in all cases.

What is the motivation behind it?

I think the motivation is the same as the repository level pruning.

The “repository-level pruning” (to use your term) removes code and data that are unchanged since a previous compile. Perhaps “module-level” pruning is just an early optimization to reduce the amount of stuff going through the back-end?

Does this [module level pruning (Ed)] only happen if there is no existing repository (or simply when objects in the compilation are not found in the repo)?

I am not sure I fully understand your question. It does not matter whether there is an existing repository or not.

This seems to contradict what you said in the text that I quoted (from this comment):

Type 2 is the module level pruning.
Assuming there is no existing repository. [snip]

Did you mean to say something like “if the objects are not already present in the repository“?

MaggieYingYi · 2020-08-05T17:51:06Z

Hmm, okay. These are, however, terms that are new to me. Have I simply forgotten from them from earlier discussions (which would be distressing)?

Sorry, we did discuss this when we implement the RepoPruning pass.

Nonetheless, I think that it would be wise to separate these two changes so that the code relating to the functional change can be easily seen.

Fully agree. I will create a separate PR for code refactoring.

Perhaps “module-level” pruning is just an early optimization to reduce the amount of stuff going through the back-end?

Yes, thanks for clarifying the difference between these two kinds of pruning.

Did you mean to say something like “if the objects are not already present in the repository“?

Yes, thanks for correcting this.

MaggieYingYi · 2020-08-05T19:11:30Z

Fully agree. I will create a separate PR for code refactoring.

I have created #113 for code review, thanks.

MaggieYingYi added 2 commits July 21, 2020 15:09

Code refactoring for the repo pruning pass.

45c5e9f

Don't prune the discardable GO if it is not referenced by the pruned …

2b2f0d4

…GOs. Fix for <#102>

MaggieYingYi requested review from FlameTop, paulhuggett and rgal July 21, 2020 16:04

MaggieYingYi self-assigned this Jul 21, 2020

MaggieYingYi mentioned this pull request Jul 21, 2020

Runtime Error in `/llvm-project/build_repo/bin/clang-tblgen': double free or corruption (out): 0x00007fe778296350 #102

Open

paulhuggett linked an issue Jul 22, 2020 that may be closed by this pull request

Runtime Error in `/llvm-project/build_repo/bin/clang-tblgen': double free or corruption (out): 0x00007fe778296350 #102

Open

MaggieYingYi changed the title ~~Fixing a runtime Error in /llvm-project/build_repo/bin/clang-tblgen': double free or corruption.~~ Fixing for Discarded GOs are emitted during the second build. Aug 3, 2020

paulhuggett reviewed Aug 3, 2020

View reviewed changes

llvm/lib/Transforms/IPO/RepoPruning.cpp Outdated Show resolved Hide resolved

MaggieYingYi changed the title ~~Fixing for Discarded GOs are emitted during the second build.~~ Fixing for Discarded GOs which are emitted during the second build. Aug 3, 2020

paulhuggett reviewed Aug 3, 2020

View reviewed changes

Updating the patch following the reviewer's comments.

3fe5390

MaggieYingYi mentioned this pull request Aug 5, 2020

Code refactoring for the repo pruning pass. #113

Merged

paulhuggett removed a link to an issue Aug 18, 2020

Runtime Error in `/llvm-project/build_repo/bin/clang-tblgen': double free or corruption (out): 0x00007fe778296350 #102

Open

paulhuggett linked an issue Aug 18, 2020 that may be closed by this pull request

Discarded GOs are emitted during the second build. #55

Open

MaggieYingYi closed this Feb 10, 2021

paulhuggett deleted the issue102 branch June 26, 2021 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing for Discarded GOs which are emitted during the second build. #109

Fixing for Discarded GOs which are emitted during the second build. #109

MaggieYingYi commented Jul 21, 2020 •

edited

Loading

paulhuggett left a comment

paulhuggett Aug 3, 2020

MaggieYingYi Aug 5, 2020

paulhuggett Aug 3, 2020

MaggieYingYi Aug 5, 2020

paulhuggett Aug 5, 2020

MaggieYingYi Aug 5, 2020

MaggieYingYi commented Aug 4, 2020

paulhuggett commented Aug 5, 2020

MaggieYingYi commented Aug 5, 2020

paulhuggett commented Aug 5, 2020

MaggieYingYi commented Aug 5, 2020

MaggieYingYi commented Aug 5, 2020

Fixing for Discarded GOs which are emitted during the second build. #109

Fixing for Discarded GOs which are emitted during the second build. #109

Conversation

MaggieYingYi commented Jul 21, 2020 • edited Loading

paulhuggett left a comment

Choose a reason for hiding this comment

paulhuggett Aug 3, 2020

Choose a reason for hiding this comment

MaggieYingYi Aug 5, 2020

Choose a reason for hiding this comment

paulhuggett Aug 3, 2020

Choose a reason for hiding this comment

MaggieYingYi Aug 5, 2020

Choose a reason for hiding this comment

paulhuggett Aug 5, 2020

Choose a reason for hiding this comment

MaggieYingYi Aug 5, 2020

Choose a reason for hiding this comment

MaggieYingYi commented Aug 4, 2020

paulhuggett commented Aug 5, 2020

MaggieYingYi commented Aug 5, 2020

paulhuggett commented Aug 5, 2020

MaggieYingYi commented Aug 5, 2020

MaggieYingYi commented Aug 5, 2020

MaggieYingYi commented Jul 21, 2020 •

edited

Loading