On Mon, Oct 21, 2019 at 5:26 AM Andrew Burgess <andrew.burg...@embecosm.com> wrote: > Below is a new versions of this patch, I believe that this addresses > the review comments from the earlier version. In addition this has > been tested using Jim's idea of forcing -msave-restore (if the flag is > not otherwise given) and I now see no test failures for newlib and > linux toolchains. > > One thing that has been mentioned briefly was adding a flag to control > just this optimisation, I haven't done that yet, but can if that is > required - obviously this optimisation doesn't do anything if > -msave-restore is turned off, so that is always an option. With no > test failures I don't know if a separate flag is required.
I can live without the flag to control it, since as you mention -msave-restore will turn it off. I'm seeing failures with the new save-restore-4.c testcase for 32-bit targets. It works for 64-bit but not for 32-bit, because the sign-extension it is checking for only happens for 64-bit targets. The testcase should check for a target of riscv64*-*-* or else hard code -march=rv64gc etc options. Either fix seems reasonable. You fixed some British spellings of optimise to optimize, but there are two new comments that add two more uses of optimise which is inconsistent. I also noticed a "useage" that should be "usage". Otherwise this looks good to me. It is OK to check in with the above minor issues fixed. Jim
