[PATCH] D84833: Implement indirect branch generation in position independent code for the RISC-V target and reorder BranchRelaxation to be run after RISCVExpandPseudo

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 requested changes to this revision.
jrtc27 added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:35-37
+static cl::opt<bool>
+    BranchRelaxation("riscv-enable-branch-relax", cl::Hidden, cl::init(true),
+                     cl::desc("Relax out of range conditional branches"));
----------------
Do we really want to be able to turn this off? It's only used when needed, and I don't see why you'd ever want to force code to fail to compile that could easily be fixed up by the compiler to work. Do note that the relaxation pass does double-duty, doing *both* direct -> indirect relaxation (rare) but *also* conditional direct -> unconditional direct relaxation which isn't that rare given the 13-bit signed displacement available for conditional branches (compared with JAL's 21-bit signed displacement). For example, I know FreeBSD kernels hits the latter case in a few places. Though even if this option only affected the indirect relaxation I'd still argue your compiler shouldn't provide an option that does nothing to affect CodeGen other than artificially stopping it from compiling certain programs.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:182-184
+  // The BranchRelaxation pass must be run after the RISCVExpandPseudo pass
+  // because it may generate additional instructions,
+  // resulting in out-of-range branches that are not relaxed.
----------------
This should not matter. All the pseudoinstructions are supposed to have correct lengths for their expanded forms (see https://reviews.llvm.org/D77443), so please revert this hunk. That also means you can use PseudoLLA in insertIndirectBranch rather than having to duplicate the expansion of it.


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

https://reviews.llvm.org/D84833