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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:444
+    assert(N == 1 && "Invalid number of operands!");
+    const MCExpr *Expr = getImm();
+    addExpr(Inst, Expr);
----------------
koakuma wrote:
> 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?
Roughly:
* Add a new enum member to KindTy, e.g. k_ASITag
* Make isASITag return true if the kind is k_ASITag
* Add a new member to the anonymous union in SparcOperand that will hold the ASI tag number (unsigned integer probably)
* Add SparcOperand::CreateASITag accepting ASI tag number, use this method in parseASITag instead of CreateImm
* Update SparcOperand::print and addASITagOperands accordingly



================
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) ||
----------------
koakuma wrote:
> 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.
I'd personally prefer parseExpression() instead of parseSparcAsmOperand() if the first token is not '#'. Up to you.



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1233
+        }
+        Operands.push_back(std::move(Op));
+      }
----------------
koakuma wrote:
> barannikov88 wrote:
> > Missing return?
> > 
> Should be no problem since it'll fall through the if check below it, but will anyway for clarity, thanks.
Actually there was a problem because parseSparcAsmOperand consumes tokens :)
I think there is another issue now -- what happens if there was a '%' but parseSparcAsmOperand didn't return success?



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1237
+
+    if (getLexer().is(AsmToken::Integer) || getLexer().is(AsmToken::LParen) ||
+        getLexer().is(AsmToken::Hash))
----------------
koakuma wrote:
> 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?
> Because using a constant expression for ASI identifier value (for example `ldxa [%o0] (0x80 + 0x08), %o1`) is also legal.

That's weird. Can you add a couple of testcases using this syntax?

> Is there a better way to detect/process it?

I think not.



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

https://reviews.llvm.org/D144936