Re: Re: Discuss the RVC implementation

[Xiaolin Zheng]

Hi Felix,
Nice to reach a consensus!
Seems loom's backend code only needs to adapt with the post-call nop. So they are orthogonal with just a little overlapping. Therefore, whichever merged secondly could do that minor adjustment based on the former one.
(Sorry for the late reply for being on holiday without opening my mailbox)
Regards,
Xiaolin
------------------------------------------------------------------
From:yangfei <yangfei at iscas.ac.cn>
Send Time:2022年10月4日(星期二) 15:39
To:郑孝林(云矅) <yunyao.zxl at alibaba-inc.com>
Cc:riscv-port-dev <riscv-port-dev at openjdk.org>
Subject:Re: Re: Discuss the RVC implementation
Hi Xiaolin,
Thanks for the thorough consierations. Comments inlined.
> Hi Felix,
>
> Thank you for taking the time to consider this, and the discussions.
>
> I think it's certainly a fairly good observation, regarding the three versions that can theoretically cover any case in combination, in an instruction-level granularity. But in reality, I may have some of my personal practices to share: such might be too fine-grained to implement a high-level control, please let me explain it.
>
> Let alone correctness, there are also code styles and maintenance that we have to focus on for sure. For example, if we want to rewrite one piece of code[1] with a fixed length by removing the `IncompressibleRegion` thing, to an instruction-level granularity, it might become [2]. Please see my comments in that gist.
>
> 1. From the code style aspect:
> We can see it is not looking so promising. In fact, my RVC prototype was in exactly the same way as your thought (so I guess it might be an intuitive and general thought :-) ), in an instruction-level granularity. And I sadly found the code style was messy even to myself. We have to overload lots of things such as _ld(Register, Address), _ld(Register, address), (see my comments) and so on to fulfill any usage in an incompressible piece of code: the overall API changes (like _ld in any form) are not convergent.
>
> In the comments from the gist, we can see we certainly have to make incompressible all the callees, even the callees of the callees, and so on, in a transitive relation. For example, the 'la(Register, Address)' API itself must be incompressible if we are in an instruction granularity. So we have to make its callee, 'la(Register, address)' API incompressible as well, and so on. It might be indeed an inferno...
Yes, I think the 'la' case here already demonstrates the complexity of my proposal. I agree an 'IncompressibleRegion' mark would be simpler and easier for the developers.
> 2. From the compression rate aspect:
> Besides, we are just talking about la() here. If we directly mark la()s as incompressible, then the la()s called by actually safe and compressible code will be left as incompressible forever. The compression rate will be definitely lower: the main issue here is, of course, the granularity problem -- instruction-level granularity is too fine-grained, which cannot allow us to make high-level controls.
>
> The current `CompressibleRegion` combined with `IncompressibleRegion` can implement a function-level granularity (neither too fine nor too coarse), which I think is very suitable for the current backend, that we can use them combined to mark everything without many efforts and with a concentration (like the current implementation: the unified relocate() with a lambda[3] and an IncompressibleRegion hidden inside). With them both, we can avoid the above problems with no effort, please see the first line of [1]: the incompressible region directly controls the current function, marking THE 'la' it currently uses incompressible, without affecting the 'la' definitions themselves (movptr, ld ... are as well). So we can avoid lots of invasions to the current backend code base. Nice, right?
I have went through your local changes about unified relocate() with a lambda. And I think it looks better and we can go on with this solution if no objections.
> 3. From the maintenance aspect:
Explicitly adding '_' to every compressible instruction might be a burden for developers and porters. One may say, just adding some '_'s, why burdens? In fact, considering we are porting code like [1] again from AArch64 port. We not only have to change instructions to RISC-V's, but also have to consider RVC... does one instruction have '_' or not? Do its callees, even its callees' callees, have an incompressible version? Even if to myself, it might be a heavy burden :-) I might feel very troublesome - I may just want to ctrl+c and ctrl+v some code without other confusion. So, why not directly throw an `IncompressionRegion` to that stub with a fixed length, so that programmers can normally write their code with the normal "ld", "la" and "addi"? Everything is easily solved without caring for the trifling :-)
That makes sense :-)
> Just sharing some practices from the same thought and might be verbose again -- there are things not easy to foresee at a glance. When implementing, the pitfalls might be obvious then. From my personal perspective, I may consider the CompressibleRegions plan looks better though, and I am looking forward to your views and suggestions.
>
>
> Best,
> Xiaolin
>
>
> [1] https://github.com/zhengxiaolinX/jdk/blob/2ee3204ace5a7767482819be2240982cc0744f8c/src/hotspot/cpu/riscv/gc/shared/barrierSetAssembler_riscv.cpp#L196-L275 <https://github.com/zhengxiaolinX/jdk/blob/2ee3204ace5a7767482819be2240982cc0744f8c/src/hotspot/cpu/riscv/gc/shared/barrierSetAssembler_riscv.cpp#L196-L275 >
> [2] https://gist.github.com/zhengxiaolinX/3151db356a9001f58827d272c8330bb7 <https://gist.github.com/zhengxiaolinX/3151db356a9001f58827d272c8330bb7 >
> [3] https://github.com/zhengxiaolinX/jdk/blob/2ee3204ace5a7767482819be2240982cc0744f8c/src/hotspot/cpu/riscv/assembler_riscv.hpp#L2167-L2178 <https://github.com/zhengxiaolinX/jdk/blob/2ee3204ace5a7767482819be2240982cc0744f8c/src/hotspot/cpu/riscv/assembler_riscv.hpp#L2167-L2178 >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/riscv-port-dev/attachments/20221008/0910542e/attachment-0001.htm>