[PATCH] D19584: AMDGPU/SI: Assembler: Unify parsing/printing of operands.

[Sam Kolton via llvm-commits]

SamWot added a comment.

LGTM. Some minor comments.
This might be better to split this in several patches.


================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:74-75
@@ -70,4 +73,4 @@
     ImmTyTFE,
-    ImmTyClamp,
-    ImmTyOMod,
+    ImmTyClampSI,
+    ImmTyOModSI,
     ImmTyDppCtrl,
----------------
Why do we need to rename Clamp and OMod?

================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:550
@@ -529,1 +549,3 @@
+                                          int64_t Default = 0, bool AddDefault = false,
+                                          bool (*ConvertResult)(int64_t&) = 0);
   OperandMatchResultTy parseNamedBit(const char *Name, OperandVector &Operands,
----------------
Consider using std::function instead of function pointer

================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:1156-1157
@@ -1122,2 +1155,4 @@
   // Try to parse with a custom parser
+  if (Mnemonic.endswith("_e64")) { Mnemonic = Mnemonic.substr(0, Mnemonic.size() - 4); }
+  if (Mnemonic.endswith("_e32")) { Mnemonic = Mnemonic.substr(0, Mnemonic.size() - 4); }
   OperandMatchResultTy ResTy = MatchOperandParserImpl(Operands, Mnemonic);
----------------
This better to be placed in ParseInstruction() method

================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:1953
@@ -1974,6 +1952,3 @@
 
-  if (Operands.size() > 3) {
-    AMDGPUOperand &Src1Op = ((AMDGPUOperand&)*Operands[3]);
-    if (Src1Op.isRegClass(AMDGPU::SReg_32RegClassID) ||
-        Src1Op.isRegClass(AMDGPU::SReg_64RegClassID))
-      return true;
+AMDGPUAsmParser::OperandMatchResultTy AMDGPUAsmParser::parseOptionalOperand(OperandVector &Operands, const OptionalOperand& Op, bool AddDefault)
+{
----------------
Support for sdwa should be added here. It might be done later.

================
Comment at: lib/Target/AMDGPU/SIInstrInfo.td:487-493
@@ -609,9 +486,9 @@
 
 class NamedBitMatchClass<string BitName> : AsmOperandClass {
   let Name = "Imm"#BitName;
   let PredicateMethod = "is"#BitName;
   let ParserMethod = "parse"#BitName;
   let RenderMethod = "addImmOperands";
   let IsOptional = 1;
 }
 
----------------
This should be moved up before NamedOperandBit class.

================
Comment at: lib/Target/AMDGPU/SIInstrInfo.td:500
@@ -621,8 +499,3 @@
 
-def HwregMatchClass : AsmOperandClass {
-  let Name = "Hwreg";
-  let PredicateMethod = "isHwreg";
-  let ParserMethod = "parseHwregOp";
-  let RenderMethod = "addImmOperands";
-}
+def sdwa_sel : NamedMatchClass<"SDWASel">;
 
----------------
This should be moved up before NamedOperandBit class.


http://reviews.llvm.org/D19584