[LLVM-Reduce] - Distinct Metadata Reduction #96072
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
You can test this locally with the following command:darker --check --diff -r 4d6b9921b3801709dca9245b5b4d7c17944a036f...211101936383973e0895f8ca1570c4b52acab335 llvm/test/tools/llvm-reduce/Inputs/reduce-distinct-metadata.py llvm/test/tools/llvm-reduce/Inputs/remove-metadata.pyView the diff from darker here.--- reduce-distinct-metadata.py 2024-06-19 13:48:15.000000 +0000
+++ reduce-distinct-metadata.py 2024-06-20 10:12:10.486785 +0000
@@ -3,17 +3,17 @@
import sys
import re
input = open(sys.argv[1], "r").read().splitlines()
-depth_map = {"0": 1, "1": 3, "2": 3, "3": 2, "4" : 1}
+depth_map = {"0": 1, "1": 3, "2": 3, "3": 2, "4": 1}
for i in range(len(depth_map)):
- counter = 0
- for line in input:
- if re.match(rf".*interesting_{i}.*", line) != None:
- counter += 1
- if counter != depth_map[str(i)]:
- sys.exit(1)
+ counter = 0
+ for line in input:
+ if re.match(rf".*interesting_{i}.*", line) != None:
+ counter += 1
+ if counter != depth_map[str(i)]:
+ sys.exit(1)
sys.exit(0)
|
michalpaszkowski
requested changes
Jun 20, 2024
There was a problem hiding this comment.
Hi @rbintel! Thank you for the pull request. I added some initial comments on the style below.
michalpaszkowski
approved these changes
Jul 26, 2024
We should only ever be adding uses of it, not removing it. We should always be trying to avoid invalid IR |
arsenm
reviewed
Jul 26, 2024
Comment on lines
31
to
32
| // Named metadata with simple list-like behavior, so that it's valid to remove | ||
| // operands individually. |
There was a problem hiding this comment.
We should preserve this. If we want a mode where we blindly rip operands out of anything we don't understand, probably should be a flag
…m#102637) This prevents some unnecessary conversions to/from int64_t and IntegerAttr.
Zero-initializing all of them accidentally left the last member active. Only initialize the first one.
…02749) Including unions, where this is more important.
…102753) Pointer::activate() propagates up anyway, so that is handled. But we need to call activate() in any case since the parent might not be a union, but the activate() is still needed. Always call it and hope that the InUnion flag takes care of the potential performance problems.
- Fix include guards for headers under utils/TableGen to match their paths.
Update the list of opcodes handled by the constant_fold_binop combine to match the ones that are folded in CSEMIRBuilder::buildInstr.
A dominance query of a block that is in a different function is ill-defined, so assert that getNode() is only called for blocks that are in the same function. There are two cases, where this behavior did occur. LoopFuse didn't explicitly do this, but didn't invalidate the SCEV block dispositions, leaving dangling pointers to free'ed basic blocks behind, causing use-after-free. We do, however, want to be able to dereference basic blocks inside the dominator tree, so that we can refer to them by a number stored inside the basic block.
This patch fixes: clang/lib/Serialization/ASTReader.cpp:11484:13: error: unused variable '_' [-Werror,-Wunused-variable]
The use of _ requires either: - (void)_ and curly braces, or - [[maybe_unused]]. For simple repetitions like these, we can use traditional for loops for readable warning-free code.
They don't have a body and we need to implement them ourselves. Use the Memcpy op to do that.
-fsized-deallocation was recently made the default for C++17 onwards (llvm#90373). While here, remove unneeded -faligned-allocation.
This script helps the release managers merge backport PR's. It does the following things: * Validate the PR, checks approval, target branch and many other things. * Rebases the PR * Checkout the PR locally * Pushes the PR to the release branch * Deletes the local branch I have found the script very helpful to merge the PR's.
I tried to add a limit to number of blocks visited in the paths()
function but even with a very high limit the transformation coverage was
being reduced.
After looking at the code it seemed that the function was trying to
create paths of the form
`SwitchBB...DeterminatorBB...SwitchPredecessor`. This is inefficient
because a lot of nodes in those paths (nodes before DeterminatorBB)
would be irrelevant to the optimization. We only care about paths of the
form `DeterminatorBB_Pred DeterminatorBB...SwitchBB`. This weeds out a
lot of visited nodes.
In this patch I have added a hard limit to the number of nodes visited
and changed the algorithm for path calculation. Primarily I am
traversing the use-def chain for the PHI nodes that define the state. If
we have a hole in the use-def chain (no immediate predecessors) then I
call the paths() function.
I also had to the change the select instruction unfolding code to insert
redundant one input PHIs to allow the use of the use-def chain in
calculating the paths.
The test suite coverage with this patch (including a limit on nodes
visited) is as follows:
Geomean diff:
dfa-jump-threading.NumTransforms: +13.4%
dfa-jump-threading.NumCloned: +34.1%
dfa-jump-threading.NumPaths: -80.7%
Compile time effect vs baseline (pass enabled by default) is mostly
positive:
https://llvm-compile-time-tracker.com/compare.php?from=ad8705fda25f64dcfeb6264ac4d6bac36bee91ab&to=5a3af6ce7e852f0736f706b4a8663efad5bce6ea&stat=instructions:u
Change-Id: I0fba9e0f8aa079706f633089a8ccd4ecf57547ed
Copying a triple is cheap, but not free, so let's not do that if there's no reason to do so. Trivial cleanup.
This enables the use of the more efficient dominator tree node access.
We were using tryGetRealPathName in certain places, which resolves symlinks (sometimes). This was resulting in discrepancies in behavior, depending on how a file was first reached. This path migrates all usages of tryGetRealPathName to regular getName instead. This implies one backward incompatible change for header-filtering. Our ignore-header option used to filter against suffixes of absolute paths, whereas now filter can receive working-directory relative paths in some cases, possibly braking existing filters. Chances of really braking users is pretty low: - We'll still filter against absolute paths when header is outside the working directory (e.g. /usr/bin/include/some/linux/header.h.) - Most projects run builds in a working directory that's nested inside the repository, hence relative paths still contain all the segments relative to repository root and anything else is unlikely to be meaningful. e.g. if a header is in `$HOME/work/llvm-project/clang-tools-extra/header.h` with builds being run in `$home/work/llvm-project/build`, we'll still filter against `../clang-tools-extra/header.h` which has all the useful segments as a suffix. - This is also a change in how we handle symlinks, but this is aligned with what we do in rest of our tools (clangd, tidy checks etc.). We tend to not resolve any symlinks for the file.
We are using `has_initialize` to check the class has `initialize` function instead of the `getOperationName` function.
* Add a distinct metadata reduction pass, which traverses the whole unnamed metadata tree and applies reduction where possible. Previous version could do this only partially, either removing named metadata, metadata attached to instructions or debug information. * Modify current named node operand reduction, make it more aggressive by generalizing the algorithm instead of reducing hard-coded instructions, I see no issue in trying a more aggressive reduction and rolling it back in case it doesn't go through. * Refactor some of the tests to suit new functionality: - Remove the --abort-on-invalid-reduction flag from remove-dp-values.ll tests, if it is included, the new named metadata reduction algorithm will fail at some point, if not, the test passes, valid IR is generated and the module is reduced, which costs a few more iterations. - Refactor remove-metadata.ll, the new functionality will not only erase the top level nodes but also their children nodes, so one can't expect them to be present after the run. - Refactor remove-named-metadata.ll, the expected behaviour now is to also remove the operands of !some.unknown.named. - Add a test for the new functionality, expected behaviour is to leave no boring nodes and all interesting nodes.
--aggressive-md flag. Addressed the issues pointed out by arsenm.
…llvm-project into barinov-reduce-distinct
under the --aggressive-md flag.
rbintel
requested review from
DeinAlptraum,
daniel-grumberg,
cyndyishida,
aaupov,
maksfb,
rafaelauler,
ayermolo,
dcci,
Endilll and
a team
as code owners
|
What's going on with this? The last batch of comments weren't addressed and it was closed without comment? |
|
@arsenm Closed this pull request because of a bad rebase. Will reopen another one with code review fixes and a clean commit history. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a distinct metadata reduction pass, which traverses the whole unnamed metadata tree and applies reduction where possible. Previous version could do this only partially, either removing named metadata, metadata attached to instructions or debug information.
Modify current named node operand reduction, make it more aggressive by generalizing the algorithm instead of reducing hard-coded instructions, I see no issue in trying a more aggressive reduction and rolling it back in case it doesn't go through.
Refactor some of the tests to suit new functionality:
Remove the --abort-on-invalid-reduction flag from remove-dp-values.ll tests, if it is included, the new named metadata reduction algorithm will fail at some point, if not, the test passes, valid IR is generated and the module is reduced, which costs a few more iterations.
Refactor remove-metadata.ll, the new functionality will not only erase the top level nodes but also their children nodes, so one can't expect them to be present after the run.
Refactor remove-named-metadata.ll, the expected behaviour now is to also remove the operands of !some.unknown.named.
Add a test for the new functionality, expected behaviour is to leave no boring nodes and all interesting nodes.