[12] RFR 8206895: aarch64: rework error-prone cmp instruction

[Andrew Haley]

On 07/11/2018 05:08 PM, Boris Ulasevich wrote:
> 
> On 11.07.2018 18:48, Andrew Haley wrote:
>> On 07/11/2018 04:42 PM, Boris Ulasevich wrote:
>>> Please review the following patch:
>>> http://cr.openjdk.java.net/~bulasevich/8206895/webrev.02
>>> https://bugs.openjdk.java.net/browse/JDK-8206895
>>>
>>> cmp instruction overloading introduced to force user to specify
>>> immediate value type implicitly. The only allowed type is unsigned char.
>>
>> But how does this solve the problem?  Won't the C++ compiler silently
>> truncate cmp(reg, int) to a uint8 ?
> 
> With overloaded cmp function compiler complains on any cmp(reg, imm) 
> usage. The only way to use cmp is to implicitly specify parameter type. 
> Once user specified unsigned char type (other types are not allowed), it 
> is user responsibility to ensure the value is correct:
> 
> cmp(reg, 8);    // gcc: call of overloaded cmp is ambiguous
> cmp(reg, 7722); // gcc: call of overloaded cmp is ambiguous
> cmp(reg, (u1)8);    // ok
> cmp(reg, (u1)7722); // user error

That's fine, then.

>>> For immediate values outside of byte range subs macro should be called
>>> directly.
>>>
>>> cmp calls all over the codes was fixed accordingly to the following plan:
>>>     cmp(reg, imm8)  -> cmp(reg, (u1)imm8)
>>>     cmp(reg, imm12) -> subs(zr, reg, imm12)
>>>     cmp(reg, imm64) -> subs(scratch, reg, imm64)
>>>
>>> The change was tested with jtreg tests.
>>
>> This hunk looks wrong:
>>
>> @@ -1129,7 +1129,7 @@
>>
>>     if (super_check_offset.is_register()) {
>>       br(Assembler::EQ, *L_success);
>> -    cmp(super_check_offset.as_register(), sc_offset);
>> +    subs(zr, super_check_offset.as_register(), sc_offset);
>>
>> cmp with register is still allowed.
>   Yes, cmp(reg,reg) is Ok. But sc_offset is immediate value:
> int sc_offset = in_bytes(Klass::secondary_super_cache_offset());

OK, thanks.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671