On Tue, Oct 13, 2020 at 3:09 AM Kito Cheng <kito.ch...@sifive.com> wrote:
> - The behavior of -mcpu basically equal to -march plus -mtune, but it
> has lower priority than -march and -mtune.
This looks OK to me.
I noticed a few things while testing. These don't need to be fixed
before the patch is committed.
Using an invalid cpu name results in two errors.
rohan:2116$ ./xgcc -B./ -mcpu=sifive-e51 tmp.c
cc1: error: ‘-mcpu=sifive-e51’: unknown CPU
cc1: error: unknown cpu ‘sifive-e51’ for ‘-mtune’
Ideally that should be one error. The second error may be confusing
as the user did not specify the -mtune option. Maybe riscv_parse_tune
can have another option to indicate whether it was passed a tune
string or cpu string, and only error if it was given a tune string,
since the cpu string would already have given an error. Or maybe pass
in both the cpu and tune strings, and it can ignore the cpu string if
a tune string was specified. And only give an error if we are using
the tune string. You can also avoid passing the tune string to
riscv_find_cpu which doesn't appear to be useful.
Using a valid cpu name that has different default arch than configured
gives a possibly confusing error
rohan:2117$ ./xgcc -B./ -mcpu=sifive-s51 tmp.c
cc1: error: requested ABI requires ‘-march’ to subsume the ‘D’ extension
This is for a toolchain with lp64d as default ABI. There is a
deliberate choice here not to force the ABI from the -mcpu option, but
maybe the error can be improved, as using -mabi to fix this is more
likely to be correct than using -march. Also, I never liked the use
of "subsume" here. We shouldn't force users to crack open a
dictionary to figure out what an error message means. Also, the user
didn't request an ABI, the compiler is using the configured default,
nor did the user specify a -march option. Maybe something like
cc1: error: ABI %s requires the 'D' extension, arch %s does not include it
where the %s expands to the ABi and arch that the compiler is using.
There is also another similar error that uses "subsume" to keep these
warnings consistent. If we want to fix this, this should be a
separate patch.
Jim
