[PATCH] D136088: [AArch64]SME2 instructions that use ZTO operand

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

A few minor suggestions but otherwise this looks good to me.



================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2773-2774
   if ((RegNum = MatchRegisterName(Name)))
-    return Kind == RegKind::Scalar ? RegNum : 0;
+    return Kind == RegKind::Scalar ? RegNum
+                                   : Kind == RegKind::LookupTable ? RegNum : 0;
 
----------------
Personally I think the following reads better.
```
return (Kind == RegKind::Scalar) || (Kind == RegKind::LookupTable) ? RegNum : 0;
```


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4325-4326
+
+  if (!Name.equals_insensitive("zt0"))
+    return MatchOperand_NoMatch;
+
----------------
Is this required? now that `matchRegisterNameAlias` is being used I expect its `(RegNum == 0)` check to be enough? 


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4346
+      return MatchOperand_ParseFail;
+      ;
+    }
----------------
stray `;`


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4350
+      return MatchOperand_ParseFail;
+    ;
+
----------------
stray `;`


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:5566
         "depending on the instruction, and the second immediate is immf + 3.");
+  case Match_InvalidMemoryIndexed8UImm3:
+    return Error(Loc, "index must be a multiple of 8 in range [0, 56].");
----------------
Please move this so it sits with the related `Match_InvalidMemoryIndexed` diagnostics (i.e. just before `Match_InvalidMemoryIndexed8UImm5`?)


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:6228
   case Match_InvalidMemoryIndexedRange4UImm2:
+  case Match_InvalidMemoryIndexed8UImm3:
   case Match_InvalidSVEAddSubImm8:
----------------
As above, there's an existing group for `Match_InvalidMemoryIndexed#UImm#` diagnostics.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4241-4242
+  StringRef Name = Tok.getString();
+  if (!Name.equals_insensitive("zt0"))
+    return MatchOperand_NoMatch;
+
----------------
CarolineConcatto wrote:
> CarolineConcatto wrote:
> > paulwalker-arm wrote:
> > > I think this is only necessary because you're calling `tryParseScalarRegister`, which looks to be a bit of a hack if I'm honest.
> > > 
> > > What about updating `matchRegisterNameAlias` and then calling that instead?  Something like:
> > > ```
> > > if ((RegNum = MatchRegisterName(Name)))
> > >   return Kind == RegKind::LookupTable ? RegNum : 0;
> > > ```
> > Oh..I can  use matchRegisterNameAlias. 
> > By looking others tryParse<smth> it looked like I should use tryParseScalarRegister.
> > 
> I created a RegKind::LookupTable. But don't know if that is what you meant.
> Is that correct? 
Yes, the `RegKind::LookupTable` you've created is what I had in mind.  Thanks.


================
Comment at: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4261
+            getLoc(), getContext()));
+        Lex(); // eat ]
+        return MatchOperand_Success;
----------------
CarolineConcatto wrote:
> paulwalker-arm wrote:
> > This assumes the next token is a `]`, which it might not be so you'll assemble something that is not valid syntax.  If `tryParseVectorIndex` doesn't just work, please look how it handles the closing bracket.
> It has now almost the same code as tryParseVectorIndex. 
> But for tryParseZTOperand it creates an Imm
>   Operands.push_back(AArch64Operand::CreateImm(
> and for tryParseVectorIndex it creates a vector index:
>    Operands.push_back(AArch64Operand::CreateVectorIndex(
I think we should be able to use the existing VectorIndex code with a bit of extra tweaking but for now I good with this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136088