[PATCH] D144936: [SPARC][IAS] Recognize more SPARCv9 instructions/pseudoinstructions

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added a comment.

It looks like there are several independent changes. A few smaller diffs would be much easier to review.



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:444
+    assert(N == 1 && "Invalid number of operands!");
+    const MCExpr *Expr = getImm();
+    addExpr(Inst, Expr);
----------------
It should have its own Kind and union member. Not sure if tablegen provides you with an enum, but if it does, the enum could be used.
This is "safer" and allows you to avoid converting to MCExpr and back.



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1105
+  std::unique_ptr<SparcOperand> Mask;
+  if (parseSparcAsmOperand(Mask) == MatchOperand_Success) {
+    if (!Mask->isImm() || !Mask->getImm()->evaluateAsAbsolute(ASIVal) ||
----------------
Why not just `getTok().is(AsmToken::Integer)` and `getTok().getIntVal()` on success?



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1113
+    SMLoc TagStart = getLexer().getLoc();
+    Parser.Lex(); // Eat the '#'.
+    auto ASIName = Parser.getTok().getString();
----------------
Is space after '#' allowed? If not, `getLexer().peekToken(false)` should be used.



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1132
+  EVal = MCConstantExpr::create(ASIVal, getContext());
+  SMLoc E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);
+  Operands.push_back(SparcOperand::CreateImm(EVal, S, E));
----------------
The location is not precise. It points before the //last// whitespace (there may be several).
Ideally, you would `getTok().getEndLoc()` before `Lex()`ing the indentifier or the number.



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1218
+        if (!Op->isReg() || Op->getReg() != Sparc::ASI)
+          return MatchOperand_NoMatch;
+        // Here we patch the MEM operand from [base + %g0] into [base + 0]
----------------
This should return MatchOperand_ParseFail. NoMatch implies no tokens were consumed.



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1233
+        }
+        Operands.push_back(std::move(Op));
+      }
----------------
Missing return?



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1237
+
+    if (getLexer().is(AsmToken::Integer) || getLexer().is(AsmToken::LParen) ||
+        getLexer().is(AsmToken::Hash))
----------------
Why is LParen here?


================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1603
       return true;
     }
+    if (name.equals("asi")) {
----------------
Do you know why Sparc does not use autogenerated MatchRegisterName? (ShouldEmitMatchRegisterName = 0 in Sparc.td.)



================
Comment at: llvm/lib/Target/Sparc/SparcASITags.td:16-21
+class ASITag<string name, bits<8> op> {
+  string Name = name;
+  // A maximum of one alias is supported right now.
+  string AltName = name;
+  bits<8> Encoding = op;
+}
----------------
It looks like all tags have an alternative name, just pass it as template parameter.



================
Comment at: llvm/lib/Target/Sparc/SparcInstrAliases.td:513
+let EmitPriority = 0 in
+  def : InstAlias<"or $simm13, $rs1, $rd", (ORri IntRegs:$rd, IntRegs:$rs1, i32imm:$simm13)>;
+
----------------
It should be `simm13Op`, though it does not currently matter that much as it is missing AsmOperandClass, so no overflow checking is done.



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

https://reviews.llvm.org/D144936