[FIRRTL] Make "intrinsic" name of intmodule mandatory. (#6858)

It's mandatory in the FIRRTL spec and only an error if it's missing,
so directly require this.

Add test for this and invalid intrinsic name.

include/circt/Dialect/FIRRTL/FIRRTLStructure.td

@@ -225,7 +225,7 @@ def FIntModuleOp : FIRRTLModuleLike<"intmodule"> {
     The "firrtl.intmodule" operation represents a compiler intrinsic.
   }];
   let arguments = (ins
-                   OptionalAttr<StrAttr>:$intrinsic,
+                   StrAttr:$intrinsic,
                    ArrayRefAttr:$portLocations,
                    ParamDeclArrayAttr:$parameters,
                    DefaultValuedAttr<AnnotationArrayAttr,
@@ -240,7 +240,7 @@ def FIntModuleOp : FIRRTLModuleLike<"intmodule"> {
   let builders = [
     OpBuilder<(ins "StringAttr":$name,
                       "ArrayRef<PortInfo>":$ports,
-                      CArg<"StringRef", "StringRef()">:$intrinsicNameAttr,
+                      "StringRef":$intrinsicNameStr,
                       CArg<"ArrayAttr", "ArrayAttr()">:$annotations,
                       CArg<"ArrayAttr", "ArrayAttr()">:$parameters,
                       CArg<"ArrayAttr", "ArrayAttr()">:$internalPaths,

lib/Dialect/FIRRTL/Export/FIREmitter.cpp

@@ -490,18 +490,9 @@ void Emitter::emitModule(FIntModuleOp op) {
     auto ports = op.getPorts();
     emitModulePorts(ports);
 
-    // Emit the optional intrinsic.
-    //
-    // TODO: This really shouldn't be optional, but it is currently encoded like
-    // this.
-    if (op.getIntrinsic().has_value()) {
-      auto intrinsic = *op.getIntrinsic();
-      if (!intrinsic.empty()) {
     startStatement();
-        ps << "intrinsic = " << PPExtString(*op.getIntrinsic());
+    ps << "intrinsic = " << PPExtString(op.getIntrinsic());
     setPendingNewline();
-      }
-    }
 
     // Emit the parameters.
     emitModuleParameters(op, op.getParameters());

lib/Dialect/FIRRTL/FIRRTLIntrinsics.cpp

@@ -137,11 +137,6 @@ LogicalResult IntrinsicLowerings::lower(CircuitOp circuit,
       continue;
 
     auto intname = intMod.getIntrinsicAttr();
-    if (!intname) {
-      op.emitError("intrinsic module with no intrinsic name");
-      ++numFailures;
-      continue;
-    }
 
     // Find the converter and apply it.
     auto it = intmods.find(intname);

lib/Dialect/FIRRTL/FIRRTLOps.cpp

@@ -1028,11 +1028,11 @@ void FExtModuleOp::build(OpBuilder &builder, OperationState &result,
 
 void FIntModuleOp::build(OpBuilder &builder, OperationState &result,
                          StringAttr name, ArrayRef<PortInfo> ports,
-                         StringRef intrinsicNameAttr, ArrayAttr annotations,
+                         StringRef intrinsicNameStr, ArrayAttr annotations,
                          ArrayAttr parameters, ArrayAttr internalPaths,
                          ArrayAttr layers) {
   buildModule(builder, result, name, ports, annotations, layers);
-  result.addAttribute("intrinsic", builder.getStringAttr(intrinsicNameAttr));
+  result.addAttribute("intrinsic", builder.getStringAttr(intrinsicNameStr));
   if (!parameters)
     parameters = builder.getArrayAttr({});
   result.addAttribute(getParametersAttrName(result.name), parameters);

lib/Dialect/FIRRTL/Import/FIRParser.cpp

@@ -5005,6 +5005,7 @@ ParseResult FIRCircuitParser::parseExtModule(CircuitOp circuit,
 ParseResult FIRCircuitParser::parseIntModule(CircuitOp circuit,
                                              unsigned indent) {
   StringAttr name;
+  StringRef intName;
   ArrayAttr layers;
   SmallVector<PortInfo, 8> portList;
   SmallVector<SMLoc> portLocs;
@@ -5013,15 +5014,11 @@ ParseResult FIRCircuitParser::parseIntModule(CircuitOp circuit,
   if (parseId(name, "expected intmodule name") ||
       parseOptionalEnabledLayers(layers) ||
       parseToken(FIRToken::colon, "expected ':' in intmodule definition") ||
-      info.parseOptionalInfo() || parsePortList(portList, portLocs, indent))
-    return failure();
-
-  StringRef intName;
-  if (consumeIf(FIRToken::kw_intrinsic)) {
-    if (parseToken(FIRToken::equal, "expected '=' in intrinsic") ||
+      info.parseOptionalInfo() || parsePortList(portList, portLocs, indent) ||
+      parseToken(FIRToken::kw_intrinsic, "expected 'intrinsic'") ||
+      parseToken(FIRToken::equal, "expected '=' in intrinsic") ||
       parseId(intName, "expected intrinsic name"))
     return failure();
-  }
 
   ArrayAttr parameters;
   ArrayAttr internalPaths;

test/Dialect/FIRRTL/emit-basic.mlir

@@ -785,7 +785,8 @@ firrtl.circuit "Foo" {
     layers = [
       @GroupA,
       @GroupA::@GroupB
-    ]
+    ],
+    intrinsic = "test"
   }
 
   // CHECK:      module ModuleWithLargeEnabledLayers

test/Dialect/FIRRTL/parse-errors.fir

@@ -1382,3 +1382,16 @@ FIRRTL version 4.0.0
 circuit PrivateMainModule:
   ; expected-error @below {{private main modules were removed in FIRRTL 4.0.0}}
   module PrivateMainModule:
+
+;// -----
+circuit IntModuleNoIntrinsic:
+  intmodule test:
+  ; expected-error @below {{expected 'intrinsic'}}
+  module IntModule:
+
+;// -----
+circuit IntModuleBadIntrinsic:
+  intmodule test:
+    ; expected-error @below {{expected intrinsic name}}
+    intrinsic = 0
+  module IntModule: