Implementation of a native context switch for Fiber on RISC-V#20964
Implementation of a native context switch for Fiber on RISC-V#20964denizzzka wants to merge 1 commit intodlang:masterfrom
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
| $(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.
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.
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.
$< 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.
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.
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.
Just leave ASM and .d files here, without changing the Makefile?
There was a problem hiding this comment.
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.
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.
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.
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