-
-
Notifications
You must be signed in to change notification settings - Fork 652
Implementation of a native context switch for Fiber on RISC-V #20964
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
Thanks for your pull request and interest in making D better, @denizzzka! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#20964" |
a13b1e2
to
636ec2e
Compare
@@ -393,7 +393,7 @@ $(ROOT)/errno_c$(DOTOBJ) : src/core/stdc/errno.c $(DMD) | |||
@mkdir -p $(dir $@) | |||
$(DMD) -c $(DFLAGS) -I. -P=-I. $< -of$@ | |||
|
|||
$(ROOT)/threadasm$(DOTOBJ) : src/core/thread/fiber/switch_context_asm.S | |||
$(ROOT)/threadasm$(DOTOBJ) : src/core/thread/fiber/switch_context_asm.S src/core/thread/fiber/switch_context_riscv.S |
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.
Hey Denis, first of all sorry for this PR being ignored for so long. I can't really comment on the asm itself, but I take it you've tested it, so I think that's a start at the very least.
I don't think this change here works. $<
in the recipe is the first input, so the new asm file would be ignored. You'd most likely either need a 2nd rule, or simply merge the new asm into the existing file, as for the existing archs needing asm. I know you don't like it ;) - but this is code hardly anyone will have to look at, so the file doesn't need to be super-pretty, and keeping it a single file means no build systems need to be adapted.
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.
What if just don't change this line at all?
Actually, DMD won't use this file because it doesn't compile for RISC-V, LDC uses its own CMake scripts and GDC uses another approach at all for context switching code organisation (https://github.com/gcc-mirror/gcc/tree/master/libphobos/libdruntime/config)
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.
$< in the recipe is the first input, so the new asm file would be ignored.
I checked target right now just by swapping these two .S files and druntime tests passed.
So, target $(ROOT)/threadasm$(DOTOBJ)
works with all these files
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.
No it doesn't work at all, as I said: https://github.com/dlang/dmd/actions/runs/14878562144/job/41780986635?pr=21353#step:10:78:
cc -c -m64 -fPIC -DHAVE_UNISTD_H -O3 --target=x86_64-darwin-apple src/core/thread/fiber/switch_context_riscv.S -o../generated/osx/release/64/threadasm.o
That 2nd PR of yours just proves that DMD doesn't need the current existing file at all.
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.
it is possible to add a separate target for RISC-V context switch, but there is no point in this: DMD does not use this code anyway
I suggest then canceling this PR
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.
Just leave ASM and .d files here, without changing the Makefile?
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.
Again, if you just add the RISC-V asm to the existing file, no compilers will have to change anything.
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.
GCC will do: they approach assumes splitting this ASM file into pieces for each architecture: https://github.com/gcc-mirror/gcc/tree/master/libphobos/libdruntime/config (Or maybe they wrote context switching from scratch, I don't really know.)
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.
Oh I see! @ibuclaw: Have you split up this switch_context_asm.S
for GDC? Or are your asm files any different?
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.
Some have likely diverged to support more platforms and assemblers.
Not everything in assembly is for fiber support. SystemZ has an asm implementation of get_tls_offset for example.
Same code can be tested using LDC: ldc-developers/ldc#4867