[PATCH] D23568: [RISCV 10/10] Add common fixups and relocations

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.
Herald added a subscriber: shiva0217.


================
Comment at: llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:148
+    case RISCVMCExpr::VK_RISCV_LO:
+      FixupKind = MIFrm == RISCVII::InstFormatI ? RISCV::fixup_riscv_lo12_i
+                                                : RISCV::fixup_riscv_lo12_s;
----------------
sabuasal wrote:
> sabuasal wrote:
> > I think this is problematic, fixup_riscv_lo12_i is only used for S instruction format in RISCV. I think this should be
> > if  (MIFrom == RISCVII::InstFormatI) {
> >   FixUpKind = RISCV::fixup_riscv_lo12_i;
> > } else if (RISCVII::InstFormatS) {
> >  FixUpKind = RISCV::fixup_riscv_lo12_s;
> > } else {
> >   llvm_unreacable("unexpected Instructio type for relocation  FixUpKind = RISCV::fixup_riscv_lo12_i");
> > }
> > 
> > Other wise you'll mistakenly  end up generating instructions instructions with wrong fixup kinds!
> correction: *fixup_riscv_lo12_s*  is only used for S instruction format in RISCV. 
You're right this code path isn't written very defensively, and it may start to fail if VK_RISCV_LO is used with different instruction formats. I've addressed this in rL325775. Thanks for spotting this.


Repository:
  rL LLVM

https://reviews.llvm.org/D23568