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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:428
                                            int64_t BrOffset) const {
+  unsigned XLen = STI.getTargetTriple().isArch64Bit() ? 64 : 32;
   // Ideally we could determine the supported branch offset from the
----------------
Isn't STI a RISCVSubtarget, so we can just call getXLen?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:446
+  case RISCV::PseudoJump:
+    return isIntN(32, SignExtend64((uint64_t)(BrOffset + 0x800), XLen));
   }
----------------
Nowhere else in the tree casts for uses of SignExtend64 like this (e.g. MipsMCExpr.cpp), even if strictly there could be signed integer overflow here, but where you've put the cast is too late. If you're going to cast anywhere, do it on BrOffset itself, but I'd probably just match what everyone else does and drop it althogether.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1015-1017
+let isCall = 0, isBarrier = 1, isCodeGenOnly = 0, hasSideEffects = 0,
+    isBranch = 1, isTerminator = 1,
     mayStore = 0, mayLoad = 0 in
----------------
jrtc27 wrote:
> Group the branch/terminator-related bits together and avoid unnecessary line breaks
Please combine the two short lines like I said


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

https://reviews.llvm.org/D84833