[PATCH] D92097: [RISCV] Basic jump table lowering

[Nandor Licker via Phabricator via llvm-commits]

nand added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:126
 
-  BuildMI(NewMBB, DL, TII->get(RISCV::AUIPC), DestReg)
-      .addDisp(Symbol, 0, FlagsHi);
+  if (Symbol.isJTI()) {
+    BuildMI(NewMBB, DL, TII->get(RISCV::AUIPC), DestReg)
----------------
jrtc27 wrote:
> nand wrote:
> > jrtc27 wrote:
> > > Why is it that expandAuipcInstPair needs to handle this specially but there's no corresponding logic for medlow's plain LUI/ADDI?
> > Based on my understanding, `addDisp` seems to be a generic method that produces an operand with an arbitrary offset from some reference, however it doesn't seem like offsets into jump tables are supported. Since the offset is always 0, the jump table index is handled separately. I might be wrong though.
> Right, then surely the correct fix is to add an MO_JumpTableIndex case to addDisp that calls addJumpTableIndex?
`addDisp` is common to all architectures, so I am reluctant to change it. Furthermore, `addDisp` takes an offset operand and the only sensible value for jump tables is 0. I think handling jump tables separately is nicer than asserting in `addDisp` that `off` is 0.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:265
-  // Effectively disable jump table generation.
-  setMinimumJumpTableEntries(INT_MAX);
-
----------------
lenary wrote:
> Let's make sure this doesn't have a major code size impact. I'm not sure the threshold GCC uses, something like 3 IIRC? 
> 
> The problem for code size is that the sequence to hold the jump table and the calculate the jump index is longer than a chain of ifs, if there aren't too many entries.
I am using this backend to compile OCaml in a weird way. I could try to build a bunch of OCaml binaries with various thresholds, report the results and maybe we can make a decision for a reasonable default?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92097