RISCV: clean up target expression handling

[compnerd]

Summary

The target specific expression handling was slightly regressed by
bbea64250f65480d787e1c5ff45c4de3ec2dcda8. This restores the proper
sub-expression evaluation to allow for constant folding within the
expression. We explicitly discard the layout and assembler when
evaluating the expression to avoid any symbolic computation and instead
using the evaluateAsRelocatable to canonicalise and constant fold
only.

We can also simplify the expression handling - none of the target
variants support symbolic difference. This simplifies the logic for
that and adds additional tests to ensure that we do not accidentally
regress here in the future.

[MaskRay]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

95

IIRC passing Layout can fold more expressions. In this case is Layout harmful?

llvm/test/MC/RISCV/expressions.s

8

Better to expand :[[#]]: to an integer. When testing parsing errors, it is useful to have precise column numbers.
Users need such information so it should be properly tested.

[compnerd]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

95

Yes; I guess I should add a comment here as well. I put it in the commit message - this is intentional to prevent any type of expression folding involving symbols. We want the symbolic differences to be preserved.

[compnerd]

Address feedback from @MaskRay

[MaskRay]

Looks great! One nit

llvm/test/MC/RISCV/expressions.s

8

The line numbers should keep using [[#@LINE-1]]` or +1.

Otherwise adding lines will cause unneeded churn.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

const MCFixup *RISCVMCExpr::getPCRelHiFixup(const MCFragment **DFOut) const {
   }
 
   return nullptr;
 }
 
 bool RISCVMCExpr::evaluateAsRelocatableImpl(MCValue &Res,
                                             const MCAsmLayout *Layout,
                                             const MCFixup *Fixup) const {
-  bool IsSymbolicDifference = false;
-  if (const auto *MBE = dyn_cast<MCBinaryExpr>(getSubExpr())) {
-    if (isa<MCBinaryExpr>(MBE->getLHS()) && isa<MCConstantExpr>(MBE->getRHS()))
-      MBE = cast<MCBinaryExpr>(MBE->getLHS());
-    IsSymbolicDifference = isa<MCSymbolRefExpr>(MBE->getLHS()) &&
-                           isa<MCSymbolRefExpr>(MBE->getRHS());
-  }
-
-  // Some custom fixup types are not valid with symbol difference expressions
-  if (IsSymbolicDifference) {
-    switch (getKind()) {
-    default:
-      break;
-    case VK_RISCV_LO:
-    case VK_RISCV_HI:
-    case VK_RISCV_PCREL_LO:
-    case VK_RISCV_PCREL_HI:
-    case VK_RISCV_GOT_HI:
-    case VK_RISCV_TPREL_LO:
-    case VK_RISCV_TPREL_HI:
-    case VK_RISCV_TPREL_ADD:
-    case VK_RISCV_TLS_GOT_HI:
-    case VK_RISCV_TLS_GD_HI:
-      return false;
-    }
-  }
-  return getSubExpr()->evaluateAsRelocatable(Res, Layout, Fixup);
+  // Explicitly drop the layout and assembler to prevent any symbolic folding in
+  // the expression handling.  This is required to preserve symbolic difference
+  // expressions to emit the paired relocations.
+  if (!getSubExpr()->evaluateAsRelocatable(Res, nullptr, nullptr))
+
+  Res =
+      MCValue::get(Res.getSymA(), Res.getSymB(), Res.getConstant(), getKind());
+  // Custom fixup types are not valid with symbol difference expressions.
+  return Res.getSymB() ? getKind() == VK_RISCV_None : true;
 }
 
 void RISCVMCExpr::visitUsedExpr(MCStreamer &Streamer) const {
   Streamer.visitUsedExpr(*getSubExpr());
 }
 
 RISCVMCExpr::VariantKind RISCVMCExpr::getVariantKindForName(StringRef name) {
   return StringSwitch<RISCVMCExpr::VariantKind>(name)

llvm/test/MC/RISCV/expressions.s

+# RUN: not llvm-mc -triple riscv32 -filetype obj %s -o /dev/null 2>&1 | FileCheck %s
+
+.Ltmp1:
+  .quad tls
+
+  lui a0, %hi(tls+0-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  lw a0, %lo(tls+0-.Ltmp1)(t0)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  lui a0, %tprel_hi(tls+0-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  add a0, a0, tp, %tprel_add(tls+0-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  addi a0, a0, %tprel_lo(tls+0-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  auipc a0, %tls_ie_pcrel_hi(tls+0-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  auipc a0, %tls_gd_pcrel_hi(tls+0-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  auipc a0, %pcrel_hi(tls-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  auipc a0, %got_pcrel_hi(tls-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+  addi a0, a0, %pcrel_lo(tls-.Ltmp1)
+# CHECK: :[[#@LINE-1]]:2: error: expected relocatable expression
+
+# tail tls+32
+# tail tls-tls
+# _ :[[#@LINE-1]]:2: error: expected relocatable expression