[PATCH] D84833: Implement indirect branch generation in position independent code for the RISC-V target

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

In D84833#2184858 <https://reviews.llvm.org/D84833#2184858>, @luismarques wrote:

> In D84833#2184824 <https://reviews.llvm.org/D84833#2184824>, @jrtc27 wrote:
>
>> I _believe_ we need:
>>
>>   isBranch = 1, isTerminator = 1, isBarrier = 1
>>
>> ?
>
> Sounds about right. (I don't know offhand in what circumstances you have different values for `isTerminator` and `isBarrier`).

My guess would be barrier implies terminator but that you can't really express that in TableGen (though maybe clever use of ? and backend tweaks could do something ad-hoc) so for simplicity you just have to specify both. The other way round certainly doesn't hold; conditional branches can terminate a basic block and fall through to the next one.

> With that change we now run into an `llvm_unreachable` in `RISCVInstrInfo::isBranchOffsetInRange`, maybe it's just implementing that missing case.

Yeah, looks like it. The easy answer is to do `isIntN(32, BrOffset)`, though that's not strictly correct on RV64 due to the implicit `+ 0x800` in the HI/LO relocations, so should really be `isIntN(32, SignExtend64(BrOffset + 0x800, XLen))` I think, and is basically what LLD does for checking the range of HI20 values (LLD also left-shifts and does an `isInt<20>` instead, but they're equivalent). The same also goes for the assertion in `insertIndirectBranch`. I won't ask why we use `isInt<X>(...)` there but `isIntN(X, ...)` in `isBranchOffsetInRange`... :)


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

https://reviews.llvm.org/D84833