Since spill and restore code of callee-saved registers has been implemented by common functions to reduce code duplication in the patch https://reviews.llvm.org/D62686, we can enable -msave-restore by default when we are optimizing for size in Oz.
|3,210 ms||x64 debian > libFuzzer.libFuzzer::fuzzer-finalstats.test|
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/SimpleTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-finalstats.test.tmp-SimpleTest
Use hasMinSize() if we want this feature enabled in -Oz.
Do we still want -mno-save-restore to override this? If so maybe we need to do this in clang driver instead.
FWIW, I think retaining the ability to have -mno-save-restore take effect is useful. So doing this in the clang driver as suggested makes sense.
At least with that this diff shows, the saving is small. I'd hope that we don't toggle -msave-restore for -Os/-Oz. Users can specify -msave-restore themselves.
Well, tiny tests like this aren't representative of real-world code, so I don't think it's fair to base it off this. Don't know what the figures are for real-world code.
Would this break shadow call stack which isn't compatible with save/restore lib calls? Or do we already check that somewhere?
Platforms like Android reserve a register x18(for AArch64). There was a patch for RISC-V that also reserved x18 for shadow callstack. https://reviews.llvm.org/D14599, https://reviews.llvm.org/D143355
Will it be better to reserve a register that doesn't conflict with -msave-restore?
I don't think so as long as the error about mixing -msave-restore with SCS was removed from RISCVFrameLowering.cpp when we switched to X3.
Seems like https://reviews.llvm.org/D149099 fixed it? cc: @paulkirth for confirmation.
I don't think there is an issue anymore. As @craig.topper pointed out the check in FrameLowering is no longer there, so AFAIK there is no longer a conflict.