RFR: 8288300: AArch64: Remove the assertion in fmovs/fmovd(FloatRegister, FloatRegister)

Andrew Haley aph at openjdk.java.net
Wed Jun 15 06:56:45 UTC 2022


On Wed, 15 Jun 2022 06:37:48 GMT, Hao Sun <haosun at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 1935:
>> 
>>> 1933:   INSN(fmovs,  0b000, 0b00, 0b000000);
>>> 1934:   INSN(fabss,  0b000, 0b00, 0b000001);
>>> 1935:   INSN(fnegs,  0b000, 0b00, 0b000010);
>> 
>> There's unnecessary whitespace here.
>> Suggestion:
>> 
>>   INSN(fmovs, 0b000, 0b00, 0b000000);
>>   INSN(fabss, 0b000, 0b00, 0b000001);
>>   INSN(fnegs, 0b000, 0b00, 0b000010);
>
> Thanks for your review.
> It's a style issue. I added one whitespace for `fmov/fabs/fneg` and the later `fcvt` so as to align the arguments to those of `fsqrt`. I saw such a style in several other sites in this header.
> 
> If you don't like it, I can remove the added whitespace. But I guess you may also want me to remove the whitespace I added for the `fcvt` below.

I see. You're right, we're not consistent about this in the file.

 There are advantages and disadvantages. On the one hand it's a little easier to read, but on the other it's more work to maintain and tends to get eroded over time.

Whitespace-only changes as part of a larger patch can be hard to review , because they ca hide the substantive changes.

Please take out the added whitespace in this patch, and submit just the substantive changes. I will think about what to do to make this file consistent.

-------------

PR: https://git.openjdk.org/jdk/pull/9163


More information about the hotspot-compiler-dev mailing list