[PATCH] D135960: [RISCV] Allow LI with symbol difference as constant

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added a comment.

In D135960#4111113 <https://reviews.llvm.org/D135960#4111113>, @jrtc27 wrote:

> This seems rather limited in use...

I agree this doesn't have a very high urgency but this feature was requested here <https://github.com/llvm/llvm-project/issues/57461>. Note that the GNU assembler also supports it.

> Also nothing's testing the error case.

Right, I forgot about that. Will add a test in the next version!



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2729
       // Just convert to an addi. This allows compatibility with gas.
-      emitToStreamer(Out, MCInstBuilder(RISCV::ADDI)
-                              .addReg(Reg)
-                              .addReg(RISCV::X0)
-                              .addExpr(Op1.getExpr()));
+      MCInst Addi = MCInstBuilder(RISCV::ADDI)
+                        .addReg(Reg)
----------------
jrtc27 wrote:
> jrtc27 wrote:
> > reames wrote:
> > > reames wrote:
> > > > Please separate this change as it's own diff so I can review it separately.  
> > > It also looks like we have the same problem in emitAuipcInstPair used for a bunch of the other cases.
> > This would probably be nicer if MCInstBuilder could take an SMLoc in its constructor and/or had a setLoc method. There are surely many cases of backends throwing this away as a result... though this is AsmParser so debug info is barely a thing.
> Oh right SMLoc not SDLoc, so not debug info at least
> Please separate this change as it's own diff so I can review it separately.

Will do!


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:56
       {"fixup_riscv_lo12_i", 20, 12, 0},
+      {"fixup_riscv_12_i", 20, 12, 0},
       {"fixup_riscv_lo12_s", 0, 32, 0},
----------------
jrtc27 wrote:
> Hm, this is the only fixup that doesn't correspond to a relocation?
I suppose it is. Is this a problem?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:430
+    }
+    return Value;
   case RISCV::fixup_riscv_lo12_s:
----------------
jrtc27 wrote:
> Don't you still need to mask this so the high bits of negative values get cleared?
Right, will fix!


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

https://reviews.llvm.org/D135960