[RISCV] Diagnose invalid second input register operand when using %tprel_add

[rogfer01]

Summary

RISCVMCCodeEmitter::expandAddTPRel asserts that the second operand must be x4/tp. As we are not currently checking this in the RISCVAsmParser, the assert is easy to trigger due to wrong assembly input.

This patch does a late check of this constraint.

An alternative could be using a singleton register class for x4/tp similar to the current one for sp. Unfortunately it does not result in a good diagnostic. Because add is an overloaded mnemonic, if no matching is possible, the diagnostic of the first failing alternative seems to be used as the diagnostic itself. This means that this case the %tprel_add is diagnosed as an invalid operand (because the real add instruction only has 3 operands).

[asb]

Good catch Roger, thanks! This looks good to me, though I wonder if it's worth being a bit more explicit in the checkPseudoAddTPRel comment.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

101

I'd consider incorporating more of the reasoning from your commit message here, so it's more self-documenting. e.g. "Enforcing this using a restricted register class for the second input operand of PseudoAddTPRel results in a poor diagnostic due to the fact 'add' is an overloaded mnemonic."

[rogfer01]

ChangeLog:

• Improve comment with wording kindly suggested by Alex Bradbury

[rogfer01]

Thanks a lot for the review Alex!

[jrtc27]

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

1701 ↗

(On Diff #194695)

This is rather nasty, as it currently implicitly falls through to the end of the switch block rather than with an explicit break, so any new cases need to either come before this one or add the break in. Could you please add the break in a follow-up commit?

[MaskRay]

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

1701 ↗

(On Diff #194695)

Done in r364746.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

class RISCVAsmParser : public MCTargetAsmParser {
 
   // Helper to emit pseudo instruction "la" used in GOT/PC-rel addressing.
   void emitLoadAddress(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out);
 
   // Helper to emit pseudo load/store instruction with a symbol.
   void emitLoadStoreSymbol(MCInst &Inst, unsigned Opcode, SMLoc IDLoc,
                            MCStreamer &Out, bool HasTmpReg);
 
+  // Checks that a PseudoAddTPRel is using x4/tp in its second input operand.
+  // Currently we can't give a meaningful diagnostic if we enforce this
+  // constraint earlier.
+  bool checkPseudoAddTPRel(MCInst &Inst, OperandVector &Operands);
+
   /// Helper for processing MC instructions that have been successfully matched
   /// by MatchAndEmitInstruction. Modifications to the emitted instructions,
   /// like the expansion of pseudo instructions (e.g., "li"), can be performed
   /// in this method.
-  bool processInstruction(MCInst &Inst, SMLoc IDLoc, MCStreamer &Out);
+  bool processInstruction(MCInst &Inst, SMLoc IDLoc, OperandVector &Operands,
+                          MCStreamer &Out);
 
 // Auto-generated instruction matching functions
 #define GET_ASSEMBLER_HEADER
 #include "RISCVGenAsmMatcher.inc"
 
   OperandMatchResultTy parseCSRSystemRegister(OperandVector &Operands);
   OperandMatchResultTy parseImmediate(OperandVector &Operands);
   OperandMatchResultTy parseRegister(OperandVector &Operands,
bool RISCVAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
   MCInst Inst;
 
   auto Result =
     MatchInstructionImpl(Operands, Inst, ErrorInfo, MatchingInlineAsm);
   switch (Result) {
   default:
     break;
   case Match_Success:
-    return processInstruction(Inst, IDLoc, Out);
+    return processInstruction(Inst, IDLoc, Operands, Out);
   case Match_MissingFeature:
     return Error(IDLoc, "instruction use requires an option to be enabled");
   case Match_MnemonicFail:
     return Error(IDLoc, "unrecognized instruction mnemonic");
   case Match_InvalidOperand: {
     SMLoc ErrorLoc = IDLoc;
     if (ErrorInfo != ~0U) {
       if (ErrorInfo >= Operands.size())
void RISCVAsmParser::emitLoadStoreSymbol(MCInst &Inst, unsigned Opcode,
   unsigned SymbolOpIdx = HasTmpReg ? 2 : 1;
   unsigned TmpRegOpIdx = HasTmpReg ? 1 : 0;
   MCOperand TmpReg = Inst.getOperand(TmpRegOpIdx);
   const MCExpr *Symbol = Inst.getOperand(SymbolOpIdx).getExpr();
   emitAuipcInstPair(DestReg, TmpReg, Symbol, RISCVMCExpr::VK_RISCV_PCREL_HI,
                     Opcode, IDLoc, Out);
 }
 
+bool RISCVAsmParser::checkPseudoAddTPRel(MCInst &Inst,
+                                         OperandVector &Operands) {
+  assert(Inst.getOpcode() == RISCV::PseudoAddTPRel && "Invalid instruction");
+  assert(Inst.getOperand(2).isReg() && "Unexpected second operand kind");
+  if (Inst.getOperand(2).getReg() != RISCV::X4) {
+    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[3]).getStartLoc();
+    return Error(ErrorLoc, "the second input operand must be tp/x4 when using "
+                           "%tprel_add modifier");
+  }
+
+  return false;
+}
+
 bool RISCVAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
+                                        OperandVector &Operands,
                                         MCStreamer &Out) {
   Inst.setLoc(IDLoc);
 
   switch (Inst.getOpcode()) {
   default:
     break;
   case RISCV::PseudoLI: {
     unsigned Reg = Inst.getOperand(0).getReg();
case RISCV::PseudoSD:
     emitLoadStoreSymbol(Inst, RISCV::SD, IDLoc, Out, /*HasTmpReg=*/true);
     return false;
   case RISCV::PseudoFSW:
     emitLoadStoreSymbol(Inst, RISCV::FSW, IDLoc, Out, /*HasTmpReg=*/true);
     return false;
   case RISCV::PseudoFSD:
     emitLoadStoreSymbol(Inst, RISCV::FSD, IDLoc, Out, /*HasTmpReg=*/true);
     return false;
+  case RISCV::PseudoAddTPRel:
+    if (checkPseudoAddTPRel(Inst, Operands))
+      return true;
   }
 
   emitToStreamer(Out, Inst);
   return false;
 }
 
 extern "C" void LLVMInitializeRISCVAsmParser() {
   RegisterMCAsmParser<RISCVAsmParser> X(getTheRISCV32Target());
   RegisterMCAsmParser<RISCVAsmParser> Y(getTheRISCV64Target());
 }

test/MC/RISCV/rv32i-invalid.s

 auipc a0, foo # CHECK: :[[@LINE]]:11: error: operand must be a symbol with a %pcrel_hi/%got_pcrel_hi modifier or an integer in the range [0, 1048575]
 auipc a0, %lo(foo) # CHECK: :[[@LINE]]:11: error: operand must be a symbol with a %pcrel_hi/%got_pcrel_hi modifier or an integer in the range [0, 1048575]
 auipc a0, %hi(foo) # CHECK: :[[@LINE]]:11: error: operand must be a symbol with a %pcrel_hi/%got_pcrel_hi modifier or an integer in the range [0, 1048575]
 auipc a0, %pcrel_lo(foo) # CHECK: :[[@LINE]]:11: error: operand must be a symbol with a %pcrel_hi/%got_pcrel_hi modifier or an integer in the range [0, 1048575]
 
 # TP-relative symbol names require a %tprel_add modifier.
 add a0, a0, tp, zero # CHECK: :[[@LINE]]:17: error: expected '%' for operand modifier
 add a0, a0, tp, %hi(foo) # CHECK: :[[@LINE]]:17: error: operand must be a symbol with %tprel_add modifier
+add a0, tp, a0, %tprel_add(foo) # CHECK: :[[@LINE]]:13: error: the second input operand must be tp/x4 when using %tprel_add modifier
 
 # Unrecognized operand modifier
 addi t0, sp, %modifer(255) # CHECK: :[[@LINE]]:15: error: unrecognized operand modifier
 
 # Use of operand modifier on register name
 addi t1, %lo(t2), 1 # CHECK: :[[@LINE]]:10: error: invalid operand for instruction
 
 # Invalid mnemonics