[PATCH] D92293: [RISCVAsmParser] Allow a SymbolRef operand to be a complex expression

Tue Dec 1 15:22:15 PST 2020

jrtc27 accepted this revision.
jrtc27 added inline comments.
This revision is now accepted and ready to land.

================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1862
+  MCFixup Fixup;
+  if (Expr->evaluateAsRelocatable(Res, nullptr, &Fixup))
+    return Res.getRefKind() == RISCVMCExpr::VK_RISCV_None;
----------------
MaskRay wrote:
> jrtc27 wrote:
> > MaskRay wrote:
> > > jrtc27 wrote:
> > > > Should we not be passing the assembler pointer? I don't remember when exactly RISC-V needs it, but RISC-V is pickier due to supporting linker relaxation.
> > > We don't need it here. Passing the pointer can enable more aggressive constant folding, but this appears to be fine for now.
> > Then we might as well? It's lying around in the target streamer's streamer (maybe elsewhere is more accessible too, this area of the MC layer gets a bit twisty-turny-maze).
> We can't provide it. MCAsmLayout is constructed in MCAssembler::Finish, a very late stage (and only when MCObjectStreamer is used). The AsmParser code should work for both -filetype=asm and -filetype=obj.
> 
> The simplification here makes sense. If we find it overly restrictive in the future, we can consider deleting it (most other targets don't have such rigid checks - but the diagnostics will become worse " operand must be a bare symbol name" -> something in the relocation emission stage
Well, if you use `getAssemblerPtr()` then you'd get back nullptr in those cases and fall back on the current behaviour, but there's also something to be said for consistency between the different ways this code can end up being called. I don't feel strongly about it, I just thought it was a nice to have.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92293