[RISCVAsmParser] Allow a SymbolRef operand to be a complex expression

[MaskRay]

Summary

So that instructions like lla a5, (0xFF + end) - 4 (supported by GNU as) can
be parsed.

Add a missing test that an operand like foo + foo is not allowed.

[jrtc27]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1862

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.

[MaskRay]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1862

We don't need it here. Passing the pointer can enable more aggressive constant folding, but this appears to be fine for now.

[jrtc27]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1862

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).

[MaskRay]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1862

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

[jrtc27]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1862

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.

[MaskRay]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1862

classifySymbolRef is a static method:) I'll lean not to use getAssemblerPtr then.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

 #include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"
 #include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/MC/MCValue.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/RISCVAttributes.h"
 #include "llvm/Support/TargetRegistry.h"
 
 #include <limits>
 
 using namespace llvm;
bool RISCVAsmParser::classifySymbolRef(const MCExpr *Expr,
                                        RISCVMCExpr::VariantKind &Kind) {
   Kind = RISCVMCExpr::VK_RISCV_None;
 
   if (const RISCVMCExpr *RE = dyn_cast<RISCVMCExpr>(Expr)) {
     Kind = RE->getKind();
     Expr = RE->getSubExpr();
   }
 
-  // It's a simple symbol reference or constant with no addend.
-  if (isa<MCConstantExpr>(Expr) || isa<MCSymbolRefExpr>(Expr))
-    return true;
-
-  const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr);
-  if (!BE)
-    return false;
-
-  if (!isa<MCSymbolRefExpr>(BE->getLHS()))
-    return false;
-
-  if (BE->getOpcode() != MCBinaryExpr::Add &&
-      BE->getOpcode() != MCBinaryExpr::Sub)
-    return false;
-
-  // We are able to support the subtraction of two symbol references
-  if (BE->getOpcode() == MCBinaryExpr::Sub &&
-      isa<MCSymbolRefExpr>(BE->getRHS()))
-    return true;
-
-  // See if the addend is a constant, otherwise there's more going
-  // on here than we can deal with.
-  auto AddendExpr = dyn_cast<MCConstantExpr>(BE->getRHS());
-  if (!AddendExpr)
-    return false;
-
-  // It's some symbol reference + a constant addend
-  return Kind != RISCVMCExpr::VK_RISCV_Invalid;
+  MCValue Res;
+  MCFixup Fixup;
+  if (Expr->evaluateAsRelocatable(Res, nullptr, &Fixup))
+    return Res.getRefKind() == RISCVMCExpr::VK_RISCV_None;
 }
 
 bool RISCVAsmParser::ParseDirective(AsmToken DirectiveID) {
   // This returns false if this function recognizes the directive
   // regardless of whether it is successfully handles or reports an
   // error. Otherwise it returns true to give the generic parser a
   // chance at recognizing it.
   StringRef IDVal = DirectiveID.getString();

llvm/test/MC/RISCV/rvi-pseudos-invalid.s

 la x1, %lo(foo) # CHECK: :[[@LINE]]:8: error: operand must be a bare symbol name
 
 sw a2, %hi(a_symbol), a3 # CHECK: :[[@LINE]]:8: error: operand must be a symbol with %lo/%pcrel_lo/%tprel_lo modifier or an integer in the range [-2048, 2047]
 sw a2, %lo(a_symbol), a3 # CHECK: :[[@LINE]]:23: error: invalid operand for instruction
 sw a2, %lo(a_symbol)(a4), a3 # CHECK: :[[@LINE]]:27: error: invalid operand for instruction
 
 # Too few operands must be rejected
 sw a2, a_symbol # CHECK: :[[@LINE]]:1: error: too few operands for instruction
+
+la a1, foo+foo # CHECK: :[[@LINE]]:8: error: operand must be a bare symbol name

llvm/test/MC/RISCV/rvi-pseudos.s

 # CHECK: auipc a2, %pcrel_hi(zero)
 # CHECK: lw  a2, %pcrel_lo(.Lpcrel_hi28)(a2)
 lw a2, zero
 
 # CHECK: .Lpcrel_hi29:
 # CHECK: auipc a4, %pcrel_hi(zero)
 # CHECK: sw  a3, %pcrel_lo(.Lpcrel_hi29)(a4)
 sw a3, zero, a4
+
+## Check that a complex expression can be simplified and matched.
+# CHECK: .Lpcrel_hi30:
+# CHECK: auipc a5, %pcrel_hi((255+a_symbol)-4)
+# CHECK: addi  a5, a5, %pcrel_lo(.Lpcrel_hi30)
+lla a5, (0xFF + a_symbol) - 4