RFR: 8256436: AArch64: Fix undefined behavior for signed right shift in assembler

[Xiaohong Gong]

On Thu, 19 Nov 2020 09:03:08 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> Right shift a signed negative value is implementation-defined in C++ (see [1]). It's better to avoid the signed right shift operations,
>> and use the unsigned right shift instead.
>> 
>> [1] https://docs.microsoft.com/en-us/cpp/cpp/left-shift-and-right-shift-operators-input-and-output?view=msvc-160&viewFallbackFrom=vs-2019
>> 
>> Tested jtreg langtools:tier1, hotspot:hotspot_all_no_apps and jdk:jdk_core, and all tests pass without new failures.
>
> src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 3212:
> 
>> 3210:     } else if (T != B && imm16 <= 32512 && imm16 >= -32768 && (imm16 & 0xff) == 0) {
>> 3211:       sh = 1;
>> 3212:       imm = (imm >> 8);
> 
> I'm very skeptical about the need for this. Every AArch64 compiler of which I'm aware treats signed right shift as well defined, and any incoming AArch64 compiler would need to do so in order to be compatible.

Hi @theRealAph , thanks for looking at this PR, and thanks for your comment here. Yes, I agree that the compilers we know like GCC/LLVM can make sure the behavior is defined on AArch64. And actually we didn't met any issues here. However, I'm not quite sure whether other compilers can guarantee it. This is just used to avoid the undefined behavior in future.  So do you think we need to fix it here? I can abandon this patch if this is not valuable. Thanks!

> src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 3219:
> 
>> 3217:     imm &= mask;
>> 3218:     f(0b00100101, 31, 24), f(T, 23, 22), f(0b11100011, 21, 14);
>> 3219:     f(sh, 13), f(imm, 12, 5), rf(Zd, 0);
> 
> imm here is a "signed immediate in the range -128 to 127," and it would be perverse to treat it as an unsigned value in the assembler.

Hi @theRealAph , thanks for the comment. `imm` here is an unsigned immediate and it has been masked with `0xff` before, which is the same with the implementation of `sf`.

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

PR: https://git.openjdk.java.net/jdk/pull/1307