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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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;
----------------
jrtc27 wrote:
> 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.
classifySymbolRef is a static method:) I'll lean not to use `getAssemblerPtr` then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92293