Re: [EXTERNAL] Re: rs6000: Fix typos in float128 ISA3.1 support

["Kewen.Lin" <linkw@linux.ibm.com>]

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