Hi Segher,
Thanks for the prompt review!
on 2021/6/23 上午2:56, Segher Boessenkool wrote:
> Hi!
>
> On Mon, Jun 21, 2021 at 05:27:06PM +0800, Kewen.Lin wrote:
>> Recently if we build gcc on Power with the assembler which doesn't
>> have Power10 support, the build will fail when building libgcc with
>> one error message like:
>>
>> Error: invalid switch -mpower10
>> Error: unrecognized option -mpower10
>> make[2]: *** [...gcc/gcc-base/libgcc/shared-object.mk:14: float128-p10.o]
>> Error 1
>
> In general, it is recommended to use a binutils of approximately the
> same age as the GCC you are trying to build. This is similar to us not
> supporting most other non-sensical configurations. An important reason
> for that is it cannot ever be tested, there are just too many strange
> combinations possible.
>
> That said :-)
>
Ah, ok! It explained why no one reported this. I only used the customized
binutils for Power10 build/testing before, will work with one new binutils
from now on. It did have more testing coverage.
>> By checking the culprit commit r12-1340, it's caused by some typos.
>
> (That is 9090f4807161.)
>
>> - fix test case used for libgcc_cv_powerpc_3_1_float128_hw check.
>
> I was confused here for a bit, "test case" usually means something in
> testsuite/, I'd call this just "test" :-)
>
>> BTW, there are some noises during regression testings due to
>> newer versions binutils, but they were identified as unrelated
>> after some checkings.
>
> Hrm, what kind of noise?
>
Some check selectors will fail without new binutils, those cases becomes
unsupported, like Power10 support checks. The location of our pre-built
binutils in our system has not only binutils but also some other tools
like newer version gdb, which caused some differences such as for guality
cases.
>> * config/rs6000/t-float128-hw(fp128_3_1_hw_funcs,
>> fp128_3_1_hw_src, fp128_3_1_hw_static_obj, fp128_3_1_hw_shared_obj,
>> fp128_3_1_hw_obj): Remove variables for ISA 3.1 support.
>
> Needs a space before the opening paren. Doesn't need a line break so
> early on that line btw.
>
Good catch, fixed.
> Just "Remove." or "Delete." is less confusing btw: what you wrote can be
> read as "Remove the variables from these declarations" or similar. And
> of course terseness is usually best in a changelog.
>
Fixed.
>> * config/rs6000/t-float128-p10-hw (FLOAT128_HW_INSNS): Append
>> macro FLOAT128_HW_INSNS_ISA3_1 for ISA 3.1 support.
>
> Don't say what it is for, just say what changed :-)
>
Fixed.
>> (FP128_3_1_CFLAGS_HW): Fix option typo.
>> * config/rs6000/float128-ifunc.c (SW_OR_HW_ISA3_1): Guarded
>> with FLOAT128_HW_INSNS_ISA3_1.
>
> "Guard", not "Guarded", all entries are written in the imperative, like,
> "Do this" or "Guard that".
>
Got it, fixed.
>> +#ifdef FLOAT128_HW_INSNS_ISA3_1
>> TFtype __floattikf (TItype_ppc)
>> __attribute__ ((__ifunc__ ("__floattikf_resolve")));
>
> I wonder if we now need TItype_ppc at all anymore, btw?
>
Sorry that I don't quite follow this question.
I think it stands for type signed int128, the function is to
convert signed int128 to float128, it would be needed here.
But I think that's not what you asked. Or you are referring
to replace this type with signed int128 without shielding it
with mode? If yes, it sounds like a question for Mike.
> Okay for trunk with the changelog slightly massaged. Thanks!
>
Thanks for catching changelog problems and the fix suggestion!
Fixed all of them accordingly and committed in r12-1738.
BR,
Kewen
