Add additional patch folder for performance patches #518
Conversation
There was a problem hiding this comment.
Small suggestion: should this repo patching infrastructure deal with the possibility of local changes being present in the checkouts, perhaps by giving up and failing if that's the case?
The repositories can end up quite messy if local changes exist, especially when applying the patches with git apply.
cmake/patch_llvm.py
Outdated
| 'To restore the original branch and stop patching, run "git am --abort".' | ||
| in p.stdout |
There was a problem hiding this comment.
This string matching might be an unstable solution, as there's no guarantee this message wont change between git versions.
In which scenarios running git am --abort wouldn't be acceptable after a an error code? Can they be detected in a different way?
There was a problem hiding this comment.
I wasn't particularly keen on this either, but there is no specific return code to check for. I've updated to check for a working directory so that it at least confirms an operation was left unresolved, which seems less brittle. Aborting where a abort wouldn't make sense doesn't seem to break anything, but I'd still prefer to avoid unnecessary noise.
| option( | ||
| APPLY_LLVM_PERFORMANCE_PATCHES | ||
| "During checkout, apply optional downstream patches to | ||
| llvm-project to improve performance." | ||
| ) |
There was a problem hiding this comment.
This option should probably be described in the comment block on top of the file.
Should this really be off by default? Users would probably want to get the perf improvements.
There was a problem hiding this comment.
The aim is that the default configuration should work out of the box. Downstream changes are additional points of failure (we would like to remove patches we already have), so I would prefer to prioritize the toolchain building first.
Release branches might be a different matter, since they use a defined and therefore stable version of LLVM.
cmake/patch_llvm.py
Outdated
| # Patch won't apply. | ||
| print(f"Unable to apply {patch_file}") | ||
| if args.restore_on_fail: | ||
| # Remove any patches that have already been applied. | ||
| while len(applied_patches) > 0: | ||
| r_patch = applied_patches.pop() | ||
| print(f"Reversing {r_patch}...") | ||
| reverse_args = [ | ||
| "git", | ||
| "apply", | ||
| "--ignore-whitespace", | ||
| "--3way", | ||
| "--reverse", | ||
| r_patch, | ||
| ] | ||
| p_check = subprocess.run( | ||
| reverse_args, cwd=args.llvm_dir, check=True | ||
| ) |
There was a problem hiding this comment.
Would it not be simpler to just remember the commit before we started applying patches, and do a git reset to that if a patch fails? One liner, and no need to keep track of applied patches.
There was a problem hiding this comment.
A reset would be simple, but would also reset all changes, including any other patch sets that were previously applied or any local changes by the user. Rewinding is more involved, but gets you back to the same state as you started with.
| parser = argparse.ArgumentParser(description=__doc__) | ||
| parser.add_argument( | ||
| "patchdir", | ||
| help="Set of patches to apply. This should be a directory containing one or more ordered *.patch files.", |
There was a problem hiding this comment.
The code as currently written requires this to be provided as an absolute path. We should either ease this requirement, or document it here.
I'm considering the scenario where the user wants to run this script to apply patches manually. The scenario as described in CMakeLists.txt has them in the root of LLVM-embedded-toolchain-for-Arm.
There was a problem hiding this comment.
I can see how the paths can get muddled since the git commands need to run in the llvm-project location, whereas the patch paths will be specified relative to the script. So I've adjusted the git arguments to use -C and convert the patch paths to absolute automatically.
The automatic patching currently only occurs on a new checkout when the dependencies are initially fetched, so there shouldn't be any existing changes to conflict with at that time. When working with a pre-existing (and potentially modified) checkout, it's left to the user to handle the patching. |
There was a problem hiding this comment.
Should we switch the picolibc and newlib patches to use this new script too, so that they work the same way? Currently it's slightly annoying that picolibc is applied as uncommitted changes, since I then have to commit them if I want to make any other changes to picolibc.
I already had in mind a followup patch which expands the use of the script, including adding a CMake option to select between |
This adds a second llvm-project patch directory, intended for performance enhancement patches which can be kept optional to avoid blocking builds if they become out of date with the main branch. A new CMake option controls whether or not to apply the patches, and a new script is included to manage the patching process to make it easier to identify when there is a problem with a patch.
This adds a second llvm-project patch directory, intended for performance enhancement patches which can be kept optional to avoid blocking builds if they become out of date with the main branch. A new CMake option controls whether or not to apply the patches, and a new script is included to manage the patching process to make it easier to identify when there is a problem with a patch.