[mlir] Change custom syntax of emitc.include op to resemble C

[simon-camp]

Summary

This changes the custom syntax of the emitc.include operation for standard includes.

[marbre]

LGTM and I can commit on your behalf. However, as Mehdi suggested this change, I would like to wait for his feedback first.

mlir/lib/Dialect/EmitC/IR/EmitC.cpp

120

nit: No trailing dot.

[jpienaar]

Looks nicer :-)

mlir/include/mlir/Dialect/EmitC/IR/EmitC.td

143

Unrelated to this change but isn't system_include more common terminology for this?

mlir/lib/Dialect/EmitC/IR/EmitC.cpp

138

Is verification needed here to ensure include was parsed?

[marbre]

mlir/include/mlir/Dialect/EmitC/IR/EmitC.td

143

Good question! I struggled when initially choosing a name for the attribute. Following cppreference here, it seemed to me that standard_include might be more appropriate as they always talk about "standard include directories" but never about system includes.

[mehdi_amini]

Nice!

mlir/test/Dialect/EmitC/ops.mlir

3

This isn't quite usual, what are you trying to catch here?

What is more common is to reparse the custom print instead to check that printer and parser are matching each other.

[marbre]

mlir/test/Dialect/EmitC/ops.mlir

3

Simon changed this after I proposed this change. It's actually adopting https://github.com/llvm/llvm-project/blob/3d48775b89cfcaa20dae9928f20410ee61bdda4c/mlir/test/Dialect/PDL/ops.mlir#L1-L3.

[mehdi_amini]

mlir/test/Dialect/EmitC/ops.mlir

3

I don't quite get the motivation actually, River is OOO for a couple of weeks, I can check with him when he'll popup online.

In the meantime (unless you can explain why) I rather leave this out.

[mehdi_amini]

mlir/test/Dialect/EmitC/ops.mlir

3

Got confirmation from River:

There were a few cases long ago where they tested for specific bugs. I don't think those make sense anymore though, and are just being cargoculted forward via copy/paste.

[marbre]

mlir/test/Dialect/EmitC/ops.mlir

3

Sry, I try to clarify. With the first mlir-opt we check if the custom form can be parsed. By adding the -mlir-print-op-generic, these

emitc.include <"test.h">
emitc.include "test.h"

get printed as

"emitc.include"() {include = "test.h", is_standard_include} : () -> ()
"emitc.include"() {include = "test.h"} : () -> ()

which than get parsed afterwards. Thus both formats get parsed. This is how I understand the motivation in the PDL test and thus we adopted this. But leaving this out is fine as well. As proposed we can parse the custom format with the second mlir-opt call by removing -mlir-print-op-generic from the first call.

[mehdi_amini]

mlir/test/Dialect/EmitC/ops.mlir

3

Thus both formats get parsed.

Yes, but what is the point? What are we testing with reparsing the generic format?

[marbre]

mlir/test/Dialect/EmitC/ops.mlir

3

Thus both formats get parsed.

Yes, but what is the point? What are we testing with reparsing the generic format?

Indeed. As discussed on Discord we will change to mlir-opt | mlir-opt.

[simon-camp]

Address review comments.

[simon-camp]

mlir/lib/Dialect/EmitC/IR/EmitC.cpp

138

With the latest revision a more explicit error message is generated.

mlir/include/mlir/Dialect/EmitC/IR/EmitC.td

 
     ```mlir
     // Custom form defining the inclusion of `<myheader>`.
-    emitc.include "myheader.h" is_standard_include
+    emitc.include <"myheader.h">
 
     // Generic form of the same operation.
     "emitc.include" (){include = "myheader.h", is_standard_include} : () -> ()
 
-    // Generic form defining the inclusion of `"myheader"`.
+    // Custom form defining the inclusion of `"myheader"`.
+    emitc.include "myheader.h"
+
+    // Generic form of the same operation.
     "emitc.include" (){include = "myheader.h"} : () -> ()
     ```
   }];

     Arg<StrAttr, "source file to include">:$include,
     UnitAttr:$is_standard_include
   );
-  let assemblyFormat = [{
-    $include attr-dict (`is_standard_include` $is_standard_include^)?
-  }];
+  let printer = [{ return ::print(p, *this); }];
+  let parser = [{ return ::parse$cppClass(parser, result); }];
   let verifier = ?;
 }
 

mlir/lib/Dialect/EmitC/IR/EmitC.cpp

   return value();
 }
 
+//===----------------------------------------------------------------------===//
+// IncludeOp
+//===----------------------------------------------------------------------===//
+
+static void print(OpAsmPrinter &p, IncludeOp &op) {
+  bool standardInclude = op.is_standard_include();
+
+  p << IncludeOp::getOperationName() << " ";
+  if (standardInclude)
+    p << "<";
+  p << "\"" << op.include() << "\"";
+  if (standardInclude)
+    p << ">";
+}
+
+static ParseResult parseIncludeOp(OpAsmParser &parser, OperationState &result) {
+  bool standardInclude = !parser.parseOptionalLess();
+
+  StringAttr include;
+  OptionalParseResult includeParseResult =
+      parser.parseOptionalAttribute(include, "include", result.attributes);
+  if (!includeParseResult.hasValue())
+    return parser.emitError(parser.getNameLoc()) << "expected string attribute";
+
+  if (standardInclude && parser.parseOptionalGreater())
+    return parser.emitError(parser.getNameLoc())
+           << "expected trailing '>' for standard include";
+
+  if (standardInclude)
+    result.addAttribute("is_standard_include",
+                        UnitAttr::get(parser.getBuilder().getContext()));
+
+  return success();
+}
+
 //===----------------------------------------------------------------------===//
 // TableGen'd op method definitions
 //===----------------------------------------------------------------------===//

mlir/test/Dialect/EmitC/ops.mlir

-// RUN: mlir-opt -verify-diagnostics %s | FileCheck %s
+// RUN: mlir-opt %s | mlir-opt | FileCheck %s
 
-"emitc.include" (){include = "test.h", is_standard_include} : () -> ()
-emitc.include "test.h" is_standard_include
+emitc.include <"test.h">
+emitc.include "test.h"
 
 // CHECK-LABEL: func @f(%{{.*}}: i32, %{{.*}}: !emitc.opaque<"int32_t">) {
 func @f(%arg0: i32, %f: !emitc.opaque<"int32_t">) {