-
Notifications
You must be signed in to change notification settings - Fork 37
[ATFE][ARM] remove -enable-inline-memcpy-ld-st patch #620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Downstream patch #406 caused build failures due to an upstream change to the ARM backend. This patch removes the support enabled by this downstream patch as I no longer think it's worth maintaining for performance improvements. Further work should be done upstream to regain any lost performance. Change-Id: I2a477d56a8a6f08aba5731434e4362b205982606
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes downstream patch #406 which enabled inline memcpy optimization using load/store instructions for ARM v7-M targets. The patch was causing build failures due to upstream ARM backend changes, and the maintainer has decided the performance benefits no longer justify the maintenance burden.
Key changes:
- Removed the
-enable-inline-memcpy-ld-stcommand-line flag and associated implementation - Deleted test file
memcpy-v7m.llthat validated the removed optimization - Removed
EmitMemcpyAsLdStmethod declaration and implementation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| llvm/test/CodeGen/ARM/memcpy-v7m.ll | Deleted entire test file that verified the inline memcpy optimization behavior |
| llvm/lib/Target/ARM/ARMSelectionDAGInfo.h | Removed EmitMemcpyAsLdSt method declaration from class interface |
| llvm/lib/Target/ARM/ARMSelectionDAGInfo.cpp | Removed command-line flag, helper method implementation, and call site for the optimization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stuij
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
This pull review modifies files outside of the |
|
In order to keep the downstream change tracking happy, I think this will need Thanks. |
Change-Id: I331bd8cc54165fa76ab8cc0999298fdceb90a7c9
Downstream patch #406 caused build failures due to an upstream change to the ARM backend. This patch removes the support enabled by this downstream patch as I no longer think it's worth maintaining for performance improvements. Further work should be done upstream to regain any lost performance.
Removes downstream issue: #405