Ad -mrelax, -mno-relax flags to enable/disable RISCV linker relaxation
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
include/clang/Driver/Options.td | ||
---|---|---|
1886 | I think we should define both -mrelax and -mno-relax flags | |
lib/Driver/ToolChains/Arch/RISCV.cpp | ||
116 | If we add both mrelax and mno-relax flags, we need to update this logic... // -mrelax is default, unless -mno-relax is specified. if(A->getOption().matches(options::OPT_mno_relax)) Relax = false; if (Relax) |
Could you please add a test? Given that the current version of D44886 enables linker relaxation by default in the backend, shouldn't -mno-relax cause -relax to be set?
Sorry, I misread D444886. Still, we should do Features.push_back("-relax") at least in the case where -mno-relax is given explicitly, meaning we don't have to rely on the backend default.
Thanks Kito. -mrelax and -mno-relax currently only affect the backend. For completeness, I think this patch needs to pass the appropriate flag to the linker depending on relax/no-relax.
Hi Alex. RISCVToolChain class does not define yet, so we can't pass --no-relax in RISCV::Linker::ConstructJob. Should we passing --no-relax when we introduce RISCVToolchain? Or there is another place we could implement passing RISCV linker flags?
lib/Driver/ToolChains/Arch/RISCV.cpp | ||
---|---|---|
130 | This part should move to the begin of the function, otherwise it never executed if Exts is empty string. |
I wonder if it would be safer to change this patch so it adds -mrelax and -mno-relax but doesn't compile with linker relaxation by default. That makes it easier to test linker relaxation support, and gives more time for testing before then flipping to -mrelax as the default.
test/Driver/riscv-features.c | ||
---|---|---|
8 | We need a another RUN line and CHECK here to determine the default whether +relax is passed or not when the user specifies neither -mrelax or -mno-relax. |
This is looking good to me, just needs an update to address this request for a test in riscv-features.c that demonstrates the default +relax/-relax setting.
Hi Alex. I added the testing line on D47127 which is the patch we turn on relaxation as default. Do you think it's ok?
I saw that. I don't mean to be too nit-picky, but there's an advantage in also testing the default in this patch - it tests the current behaviour and makes it obvious in the follow-up patch that the behaviour actually changed.
I think we should define both -mrelax and -mno-relax flags