Skip to content
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

OPAL_PATCHER_BEGIN/OPAL_PATCHER_END is broken in some cases #13014

Open
pinskia opened this issue Jan 2, 2025 · 3 comments · May be fixed by #13027
Open

OPAL_PATCHER_BEGIN/OPAL_PATCHER_END is broken in some cases #13014

pinskia opened this issue Jan 2, 2025 · 3 comments · May be fixed by #13027

Comments

@pinskia
Copy link

pinskia commented Jan 2, 2025

Forwarded from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95692 :

# define OPAL_PATCHER_BEGIN \

The inline-asm needs to clobber memory to avoid moving the END before a different restore the r2.

That is BEGIN should be:

asm volatile ("std 2, %0" : "=m" (toc_save) :: "memory");
asm volatile ("nop; nop; nop; nop; nop" ::: "memory");;

While END should be:

asm volatile ("ld  2, %0" : : "m" (toc_save) : "memory");;
@devreal
Copy link
Contributor

devreal commented Jan 2, 2025

Thanks @pinskia for tracking this (over 4 years)! Would you be able to file a PR that adds the clobber?

@pinskia
Copy link
Author

pinskia commented Jan 2, 2025

Thanks @pinskia for tracking this (over 4 years)! Would you be able to file a PR that adds the clobber?

I don't have a way to test it fully. I was just using godbolt (https://godbolt.org/) to do quick testing by looking at the generated assembly output.

@jsquyres
Copy link
Member

jsquyres commented Jan 3, 2025

@bosilca @janjust Can Nvidia do some testing here, and file a PR if it all works as expected? You're the main consumers of patcher these days.

@jsquyres jsquyres removed the Auto closed Closed by the stale bot label Jan 3, 2025
bosilca added a commit to bosilca/ompi that referenced this issue Jan 9, 2025
Prevent the compiler from reordering the instructions in the patcher by
marking the assembly code as clobbering memory.

Thanks @pinskia for the patch and for bringing the discussion on the gcc
bugzilla.

Fixes open-mpi#13014.

Signed-off-by: George Bosilca <[email protected]>
@bosilca bosilca linked a pull request Jan 9, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants