[PATCH v2] Support Intel SHA512

Jan Beulich jbeulich@suse.com
Thu Jul 20 10:37:49 GMT 2023


On 20.07.2023 10:32, Jiang, Haochen wrote:
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/i386/sha512.d
>>> @@ -0,0 +1,16 @@
>>> +#as:
>>
>> What purpose does this line (present in several of the tests) have?
>>
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/i386/sha512.s
>>> @@ -0,0 +1,13 @@
>>> +# Check 32bit SHA512 instructions
>>> +
>>> +	.allow_index_reg
>>
>> This doesn't look to be needed either.
>>
> 
> We use script to generate the testcases so these two are to fit all
> circumstances since actually script does not know what will happen
> for a new ISA. (The former is for some extra option in as the latter is
> for index reg.)
> 
> We could omit that but one thing I need to mention is that there are
> also some redundant things in all the existing testcases. If we want to
> eliminate all of them, some may need careful manual work. I am
> wondering if that is time-worthy to change all of them. Therefore, I
> propose not to omit that to keep align with all the testcases since it
> is not wrong. 

I guess H.J. was more permissive in what he allowed in. I'm concerned
of pieces in testcases which aren't relevant: It easily raises questions
of why things are there. I'd be happy to - over time - clean up that
aspect as well in the testsuite, just like I've been cleaning up other
oddities. I'd prefer if new testcases contained just what is needed in
there for the test to fulfill its purpose.

> All the other mentioned in the review has been fixed in my v3 patch
> which will be sent out later.

Thanks; looks like it wasn't marked as being v3.

Jan


More information about the Binutils mailing list