[PATCH] D117929: [XRay] Add support for RISCV

Sat Jan 22 08:30:40 PST 2022

jrtc27 added a comment.

In D117929#3263462 <https://reviews.llvm.org/D117929#3263462>, @ashwin98 wrote:

> Thank you for your feedback! I could combine the riscv32 and 64 cpp files with some xlen conditions if that will work better, but that might take a bit of a hit in terms of readability (do I explain both sleds in the comments preceding the implementation).

In one sense yes it will be slightly less readable. In another sense it actually makes it //more// readable, because seeing the XLEN-based conditions makes it clear what things are word-sized and what things are explicitly 32-bit (in the RV32 code any LW is unclear whether it's loading an int, a size_t or a `void *`). And yes, you explain both, but most of it is the same so can be combined into a single explanation, e.g. like LLD does for documenting its PLT stubs in lld/ELF/Arch/RISCV.cpp.

> I wasn't too sure about how to work around sign extension in RISCV, which you have picked up on - adding 0x800 wasn't something I thought of. Thinking about it a bit more, it makes sense, we're not adding 4096 like how I was, though it has the same effect; I'll reason it out, I'm sure carry propagation deals with it like you've mentioned. I'll update the code to reflect the same.

It's important that it's only added when computing the HI relocation. As an example, `%hi(0x81734)` (to pick a number at random that's not too boring) would be `(0x81734 + 0x800) >> 12` = `0x81f34 >> 12` = `0x81`, whereas `%hi(0x81934)` = `(0x81934 + 0x800) >> 12` = `0x82134 >> 12` = `0x82`. You can see how the adding `1 << 11` combined with right-shifting by one more results in adding one to the upper 20 bits if and only if bit 11 of the input is 1; if it's 0 there is no carry out so the only bit that's modified is bit 11, which the right shift will shift out.

> I had a related question with respect to the 64 bit sleds though - given that lui is also sign extended, we need a work around for it as well while constructing the 32 bit values, and while combining the 2 32 bit values into a 64 bit value. I have currently been getting rid of the upper 32 bits by performing a left shift followed by a right shift, but I'm sure there is a better solution to it.

That's one way of doing it, though requires more than one temporary register. RISCVMatInt's generateInstSeqImpl has an alternate sequence documented for the general case (as well as various optimised special cases) that is the same number of instructions but only needs one register. If you have multiple registers then your sequence probably performs better on an out-of-order core. Synthesising 64-bit addresses is pretty inefficient; you might prefer instead just loading from a constant pool adjacent to the code.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117929/new/

https://reviews.llvm.org/D117929