[PATCH] D54143: [WIP, RISCV] Generate address sequences suitable for mcmodel=medium

Tue Nov 6 08:31:30 PST 2018

jrtc27 added inline comments.

================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:362
+  SDValue AddrHi = getTargetNode(N, DL, Ty, DAG, Flags | RISCVII::MO_PCREL_HI);
+  SDValue AddrLo = getTargetNode(N, DL, Ty, DAG, Flags | RISCVII::MO_LO);
+  SDValue MNHi = SDValue(DAG.getMachineNode(RISCV::AUIPC, DL, Ty, AddrHi), 0);
----------------
lewis-revill wrote:
> rogfer01 wrote:
> > jrtc27 wrote:
> > > lewis-revill wrote:
> > > > jrtc27 wrote:
> > > > > jrtc27 wrote:
> > > > > > lewis-revill wrote:
> > > > > > > jrtc27 wrote:
> > > > > > > > This needs to be an `MO_PCREL_LO`, surely? (and then modified to refer to the `auipc` rather than the symbol...)
> > > > > > > I was under the impression that only the `auipc` symbol needs to be PC-relative, since:
> > > > > > > 1) The upper bits are the bits we need to prevent from overflowing
> > > > > > > 2) the operation of `auipc` is to add the upper 20 bits of the symbol //relative to the program counter// to the program counter itself, putting the result into the destination register. Essentially we've done the same operation as an `lui`, but via the PC. So the `addi` should be the same?
> > > > > > > 
> > > > > > > Also more importantly I tried that and got a 'relocation truncated to fit'...
> > > > > > > 
> > > > > > > I don't quite understand what you mean by 'modified to refer to the `auipc` rather than the symbol'? Basically the first two SDValues are just building the `%pcrel_hi(sym)` & `%lo(sym)` expressions to be used in the actual sequence.
> > > > > > No, because you still have the lower bits of the program counter added to the `%lo` value. What you're trying to do is use `auipc` to get the 4k-page and then set the lower bits to the offset in the page, but that needs an extra mask if you want to do that, which is pointless when you can add the `%pcrel_lo` instead and get the right result.
> > > > > > 
> > > > > > Example:
> > > > > > 
> > > > > > 0x10048: auipc a0, %pcrel_hi(X)
> > > > > > 0x1004c: addi a1, a0, %lo(X)
> > > > > > 
> > > > > > where X is at address 0x20072.
> > > > > > 
> > > > > > %pcrel_hi(X) at 0x48 will give us upper 20 bits of 0x20072-0x10048=0x1002a, ie 0x10000
> > > > > > Thus a0 gets set to PC+0x10000 = 0x20048
> > > > > > %lo(X) will give us 0x20072&0xFFF = 0x72
> > > > > > Thus a1 gets set to a0+0x72 = 0x200ba, not 0x20072, because we had the extra 0x48 from the low bits of PC at the time of the auipc.
> > > > > As for 'modified to refer to the auipc rather than the symbol', `%pcrel_lo` is unusual on RISC-V. You would think that you would write something like the following (the `+4` is of course extremely important):
> > > > > 
> > > > > ```
> > > > > .L0: auipc a0, %pcrel_hi(X)
> > > > >      addi a1, a0, %pcrel_lo(X+4)
> > > > > ```
> > > > > 
> > > > > But in fact that's not how you do it. On RISC-V, the pair of instructions are tied together so the linker knows which `auipc` the later `addi` is consuming, and you instead do this:
> > > > > 
> > > > > ```
> > > > > .L0: auipc a0, %pcrel_hi(X)
> > > > >      addi a1, a0, %pcrel_lo(.L0)
> > > > > ```
> > > > > 
> > > > > Note how the symbol for the `%pcrel_lo` becomes the label for the `auipc` rather than the symbol itself. This indirection is eventually removed by the linker once it has done all relaxations, and will turn into the low 12 bits of `X-.L0` ie what is normally `%pcrel_lo(X+4)`.
> > > > Thank you for the explanation, I see what you mean now. Now I have to figure out how to implement that.
> > > Look at the expansion of `lla` in the assembler, that's exactly what you're trying to do here.
> > In an earlier attempt to implement local PIC addressing I created a basic block to get the address
> > 
> > https://reviews.llvm.org/D50634
> > 
> > but this may impact negatively later optimisations that work at a basic-block unit. @efriedma suggested to add an id to the `auipc` instead. 
> > 
> > My approach used a pseudo instruction so what one needs in my case is to make sure the label is emitted by `AsmPrinter`. Under that approach (in case you want to consider this approach) it looks to me we need to extend first `MachineInstr` to be able to represent that id (e.g. zero if no id). `FunctionLoweringInfo` could be the provider of these unique ids within a function via `SelectionDAG`. Then teach `AsmPrinter` to emit a label when there is an id for a given instruction. Then we need to link the user of the id (in your case `addi`). That part is less clear to me but I'll be bold here and I believe `SelectionDAG::getTargetIndex` is enough here (with a MII flag stating that this is a pcrel_lo).
> > 
> > Unfortunately I could not progress on this, yet. Feel free to investigate this avenue for feasibility. Perhaps there is a simpler approach in your case.
> > 
> > (To the best of my knowledge, this way of referencing an earlier instruction for symbol relocations is a bit of a unique thing of RISC-V so I think we're in uncharted land within LLVM)
> > 
> > Hope this helps.
> > 
> > Regards,
> Thank you very much for the background, I should have looked this up before.. I think a pseudo instruction is the right approach, but it should probably be expanded as late as possible? I'm testing whether it would be worth modifying `RISCVExpandPseudoInsts` to cater for operations other than just atomics.
> 
> Maybe then we could still go with the basic block approach since most optimisations will be done already.
> 
> 
The latest point to do it is in `RISCVAsmPrinter` for the pseudo approach, as far as I am aware (and that's what I've been doing for my locally hacked up LLVM). I get uneasy with doing it earlier (ie by using a basic block and expanding somewhere like `RISCVExpandPseudoInsts`) in case instructions get moved around and the MBB no longer has the `auipc` as its first instruction, but if there are guarantees that that doesn't happen then that seems fine (although I fail to see the benefit from expanding such a simple instruction then, when doing it in `RISCVAsmPrinter` is almost exactly the same as what `RISCVAsmParser` is doing). Others with deeper LLVM-internals knowledge may be able to offer better advice here, but my approach does work.

Repository:
  rL LLVM

https://reviews.llvm.org/D54143