RFR: 8332265: RISC-V: Materialize pointers faster by using a temp register [v6]

[Hamlin Li]

On Fri, 24 May 2024 12:39:32 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

> > And a general question about `nativeInst_riscv.cpp` and `macroAssembler_riscv.cpp`. I saw the functions in these 2 files call each other, that make the code a bit mess to me. It's not an issue introduced in this pr. I wonder if this could be refactored? If so, I can work on it. But just in case you have easy answer already, so I don't have to do further investigation.
> 
> I agree, I would prefer having classes for the instruction where all the instruction functionality would be. As it's now the opcodes are reapted everywhere, instead it should just be in in-place, this class. And then have classes for instruction sequence where we keep all functionality gathered.

OK, let me do some further investigation to see if we can make it more readable and maintainable.

>> src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1681:
>> 
>>> 1679: }
>>> 1680: 
>>> 1681: void MacroAssembler::movptr1(Register Rd, uint64_t imm64, int32_t &offset) {
>> 
>> Original code of MacroAssembler:: movptr(...) is bit tricky at `upper -= lower;` to understand for me, and I think new MacroAssembler:: movptr2(...) uses the similar way at `lower30 - low12`.
>> I can add some comment later to help future understanding later.
>
> Yes, it's very confusing with 12 bit *signed*, imm 11 bits + 1 signed bit.
> Therefore we need to use arithmetic shift to create "12 bit sign value".
> (hence why they do not use 12ull, to signal this is an arithmetic shift)
> 
> So if we need to set bit 12 this value it will be negative plus the bit pattern we want in the other 11 bits.
> To compensate for that we need to lui() a larger value, and that value would be low30 - low12.
> If low12 is postive it's just removing those bits, otherwise we also need to add 4096 (bit 12).
> Hence low30 - -low12 would instead add to low30, the mid18 can thus be extended to 19 bits.
> 
> The 20 bit is sign bit so we must stay away from it otherwise lui() will sign extend the 19+1 imm value.
> 
> I don't think that helped, but I tried at least :)

Thanks for explanation. It also took me a while to figure it out.

Then I will try to add some comment for it later. Maybe also try to refactor movptr2() if I can find out a way to make it more clear.

>> src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1703:
>> 
>>> 1701:   assert_different_registers(Rd, tmp, noreg);
>>> 1702: 
>>> 1703:   uint32_t upper18 = (addr >> 30ull);
>> 
>> literal suffix `ull` could be removed?
>> And `uint32_t upper18 = (addr >> 30ull);` + `lui(tmp, upper18 << 12);` could be replaced with `lui(tmp, addr >> 18);`?
>
> **ull** is there to say should be a 64-bit logical shift, appereant not doing it's jobs.
> The reason it's done in two steps is first we get the values, then we adjust them for our implementation of the instructions.
> 
> Our lui() shifts down the immediate value so we must shift it up before.
> While when we patch lui we use the value without shifting:
> 
>   unsigned int upper18 = (addr >> 30ull);
>   Assembler::patch(instruction_address + (NativeInstruction::instruction_size * 0), 31, 12, (upper18 & 0xfffff)); 
> 
> 
> So it can be replaced but then we are missing the calculation why it's correct,

Thanks for explanation.

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

PR Comment: https://git.openjdk.org/jdk/pull/19246#issuecomment-2129563395
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1613498640
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1613498744