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

[Koakuma via Phabricator via llvm-commits]

koakuma marked 14 inline comments as done.
koakuma added a comment.

In D144936#4325027 <https://reviews.llvm.org/D144936#4325027>, @barannikov88 wrote:

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

Any advice on how to split it? This is basically just a bunch of instruction definitions, the complication mostly arises since some of the parsing needs to be handled on the C++ side.



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:444
+    assert(N == 1 && "Invalid number of operands!");
+    const MCExpr *Expr = getImm();
+    addExpr(Inst, Expr);
----------------
barannikov88 wrote:
> 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.
> 
Not entirely sure if I can/how to do this, any example code I can refer to?


================
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) ||
----------------
barannikov88 wrote:
> Why not just `getTok().is(AsmToken::Integer)` and `getTok().getIntVal()` on success?
> 
Same reason with the LParen matcher, this is to handle constant expressions for ASI identifier.
We want to evaluate those expressions before proceeding further.


================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1233
+        }
+        Operands.push_back(std::move(Op));
+      }
----------------
barannikov88 wrote:
> Missing return?
> 
Should be no problem since it'll fall through the if check below it, but will anyway for clarity, thanks.


================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1237
+
+    if (getLexer().is(AsmToken::Integer) || getLexer().is(AsmToken::LParen) ||
+        getLexer().is(AsmToken::Hash))
----------------
barannikov88 wrote:
> Why is LParen here?
Because using a constant expression for ASI identifier value (for example `ldxa [%o0] (0x80 + 0x08), %o1`) is also legal.
Is there a better way to detect/process it?


================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1603
       return true;
     }
+    if (name.equals("asi")) {
----------------
barannikov88 wrote:
> Do you know why Sparc does not use autogenerated MatchRegisterName? (ShouldEmitMatchRegisterName = 0 in Sparc.td.)
> 
Unfortunately no. I am still new to the backend so for these parts of the code I'm just following what the original code already does.
I guess I'll try it out and see if I could use autogenerated MatchRegisterName.
(But it's probably better to do it in a separate patch? I'm not sure)


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

https://reviews.llvm.org/D144936