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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 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)
----------------
nand wrote:
> 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.
I disagree. It's a generic method meant to take any operand one would reasonably want to use. We already use it for the very-similar constant pool index. You also don't need to assert `off` is 0; that's not done for CPIs, and if you pass in non-zero then you get what you asked for, ie `Symbol.getIndex() + off`. The only reason it's not currently supported in `addDisp` is that nobody has yet needed it, as other backends tend to just duplicate lowering for the different hooks. Just mirror the CPI code for what is basically a special case of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92097