[PATCH] D55325: [RISCV] Add assembler support for LA pseudo-instruction

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 marked an inline comment as done.
rogfer01 added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1483
+  RISCVMCExpr::VariantKind VKHi;
+  if (getContext().getObjectFileInfo()->isPositionIndependent()) {
+    SecondOpcode = isRV64() ? RISCV::LD : RISCV::LW;
----------------
jrtc27 wrote:
> rogfer01 wrote:
> > My understanding is that `.option pic` and `.option nopic` change this behaviour.
> > 
> > A quick look at `MCObjectFileInfo` class files shows that the attribute is only set during the construction of the object and can't be changed.
> > 
> > In my downstream I implemented `.option pic` and `.option nopic` which set a field of `RISCVAsmParser` that I called `IsPicEnabled` and is only used for `la`. That field is initialised in the constructor using `getContext().getObjectFileInfo()->isPositionIndependent()`. Perhaps adding a setter to `MCObjectFileInfo` might be a better approach.
> > 
> > That said, either we implement `.option pic` / `.option nopic` first or we add a `FIXME` here and implement it later.
> > 
> > Hope this makes sense.
> Indeed, you're absolutely right, and I have what sounds like nearly-identical code in my own local repository. Given that, as far as I know, `.option pic/nopic` only affects the expansion of `la`, I decided it made sense to add `la` first rather than a seemingly-pointless option. I'm not sure whether a `FIXME` is really necessary; anyone adding `.option pic/nopic` support must look for PIC-related checks, and we have all the code, it's just a case of changing the condition (unlike some cases where `FIXME`s do exist in code gen because the expansion just doesn't exist).
Agreed. The `FIXME` may not help much here. Thanks @jrtc27 !


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55325