Skip to content

[MemCpyOpt] Fix the invalid code modification for GEP #68479

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

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

kaiyan96
Copy link
Contributor

@kaiyan96 kaiyan96 commented Oct 7, 2023

Relocate the GEP modification to a later stage of the function performCallSlotOption, ensuring that the code remains unchanged if the optimization fails.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of rolling back, we should not do the change until we know the transform will succeed. The simplest way to do that would be to move this check to the end. (Setting a variable to perform the move later would also work.)

@kaiyan96
Copy link
Contributor Author

kaiyan96 commented Oct 8, 2023

Instead of rolling back, we should not do the change until we know the transform will succeed. The simplest way to do that would be to move this check to the end. (Setting a variable to perform the move later would also work.)

@nikic According to this modification(https://reviews.llvm.org/D89623), which claim that "When performing a call slot optimization to a GEP dest, it will currently usually not apply, because the GEP is directly before the memcpy and as such does not dominate the call. We should move it above the call if that satisfies the domination requirement." It seems that it is better to apply this move before some further checks.

Is it proper to do RAII for GEP inst instead of roll back?

@kaiyan96 kaiyan96 force-pushed the bugfix_memcpy_invalid_modification branch from ff0f90f to a6cd812 Compare October 8, 2023 09:37
@nikic
Copy link
Contributor

nikic commented Oct 8, 2023

Instead of rolling back, we should not do the change until we know the transform will succeed. The simplest way to do that would be to move this check to the end. (Setting a variable to perform the move later would also work.)

@nikic According to this modification(https://reviews.llvm.org/D89623), which claim that "When performing a call slot optimization to a GEP dest, it will currently usually not apply, because the GEP is directly before the memcpy and as such does not dominate the call. We should move it above the call if that satisfies the domination requirement." It seems that it is better to apply this move before some further checks.

The further checks do not depend on the instruction being moved. It just needs to be moved at some point if the transform is applied, it does not have to be at that specific place in the sequence of checks.

@kaiyan96 kaiyan96 force-pushed the bugfix_memcpy_invalid_modification branch from a6cd812 to ee996e2 Compare October 8, 2023 11:55
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kaiyan96 kaiyan96 force-pushed the bugfix_memcpy_invalid_modification branch 2 times, most recently from 602591c to 40084e5 Compare October 9, 2023 02:33
@lcvon007
Copy link
Contributor

lcvon007 commented Oct 9, 2023

Instead of rolling back, we should not do the change until we know the transform will succeed. The simplest way to do that would be to move this check to the end. (Setting a variable to perform the move later would also work.)

please update commit message too.

Relocate the GEP modification to a later stage of the function
performCallSlotOption, ensuring that the code remains unchanged
if the optimization fails.
@kaiyan96 kaiyan96 force-pushed the bugfix_memcpy_invalid_modification branch from 40084e5 to ab78cf1 Compare October 9, 2023 06:10
@nikic nikic merged commit df116d1 into llvm:main Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants