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

[James Clarke via Phabricator via llvm-commits]

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:
> 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.


================
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);
----------------
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)`.


Repository:
  rL LLVM

https://reviews.llvm.org/D54143