[mlir][emitc] Add a variable op

[marbre]

Summary

This adds a variable op, emitted as C/C++ locale variable, which can be
used if the emitc.constant op is not sufficient.

As an example, the canonicalization pass would transform

mlir
%0 = "emitc.constant"() {value = 0 : i32} : () -> i32
%1 = "emitc.constant"() {value = 0 : i32} : () -> i32
%2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
%3 = emitc.apply "&"(%1) : (i32) -> !emitc.ptr<i32>
emitc.call "write"(%2, %3) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()

into

mlir
%0 = "emitc.constant"() {value = 0 : i32} : () -> i32
%1 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
%2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
emitc.call "write"(%1, %2) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()

resulting in pointer aliasing, as %1 and %2 point to the same address.
In such a case, the emitc.variable operation can be used instead.

[marbre]

As an example, the canonicalization pass would transform

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %3 = emitc.apply "&"(%1) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%2, %3) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

into

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%1, %2) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

resulting in pointer aliasing. Here, the variable op would be helpful.

[jpienaar]

In D120098#3331073, @marbre wrote:

As an example, the canonicalization pass would transform

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %3 = emitc.apply "&"(%1) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%2, %3) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

into

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%1, %2) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

resulting in pointer aliasing. Here, the variable op would be helpful.

Could you elevate this to be part of the commit description and expand on the pains? (I think you'd want to expand in the op descriptions too what variety to use and why).

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

153

The sentence leaves me confused as to what is or isn't supported (can do basic and exoric types, so what's missing?)

157

I think here could be good to expand when one would use this via constant (and same constant side add comment that it shouldn't be assumed to be materialized, and then it seems we may also want to flag in the verifier if you want to take an address of a constant)

mlir/test/Dialect/EmitC/ops.mlir

27

This is using the generic syntax, so unclear what is being tested? (Printing and parsing generically tested elsewhere)

[marbre]

Address review comments

[marbre]

In D120098#3339041, @jpienaar wrote:

In D120098#3331073, @marbre wrote:

As an example, the canonicalization pass would transform

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %3 = emitc.apply "&"(%1) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%2, %3) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

into

func @myfunc() {
  %0 = "emitc.constant"() {value = 0 : i32} : () -> i32
  %1 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  %2 = emitc.apply "&"(%0) : (i32) -> !emitc.ptr<i32>
  emitc.call "write"(%1, %2) : (!emitc.ptr<i32>, !emitc.ptr<i32>) -> ()
  return
}

resulting in pointer aliasing. Here, the variable op would be helpful.

Could you elevate this to be part of the commit description and expand on the pains? (I think you'd want to expand in the op descriptions too what variety to use and why).

Thanks for the review @jpienaar! I have updated the commit description and expanded the op description. Please let me know if these changes are sufficient form your point of view.

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

153

The emitter currently supports integer, float, index and tuple types as well as a subset of tensor types (e.g. a static shape is needed). In addition the emitc types are of course supported. The sentence is mainly a take over from the emitc.constant operation, so I think it might be fine to leave as is?

157

I elaborated more on what kind of code this allows to write and that it is intended to be used with pointers. Refactoring the emitc.apply and emitc.constant operations (as you mentioned taking the address of a constant is not a good idea) will be addressed in a followup. I would prefer to refactor the downstream users first to not break things within an integration commit that ships all changes at once.

mlir/test/Dialect/EmitC/ops.mlir

27

I removed the test.

[jpienaar]

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

157

SGTM (perhaps add a TODO for interim)

[marbre]

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

157

Fair enough. Added TODOs to the apply and constant op.

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

let description = [{
     attribute and the EmitC opaque type.
 
     Example:
 
     ```mlir
     // Integer constant
     %0 = "emitc.constant"(){value = 42 : i32} : () -> i32
 
-    // Constant emitted as `int32_t* = NULL;`
+    // Constant emitted as `char = CHAR_MIN;`
     %1 = "emitc.constant"()
-        {value = #emitc.opaque<"NULL"> : !emitc.opaque<"int32_t*">}
-        : () -> !emitc.opaque<"int32_t*">
+        {value = #emitc.opaque<"CHAR_MIN"> : !emitc.opaque<"char">}
+        : () -> !emitc.opaque<"char">
     ```
   }];
 
   let arguments = (ins AnyAttr:$value);
   let results = (outs AnyType);
 
   let hasFolder = 1;
   let hasVerifier = 1;
def EmitC_IncludeOp
   }];
   let arguments = (ins
     Arg<StrAttr, "source file to include">:$include,
     UnitAttr:$is_standard_include
   );
   let hasCustomAssemblyFormat = 1;
 }
 
+def EmitC_VariableOp : EmitC_Op<"variable", []> {
+  let summary = "Variable operation";
+  let description = [{
+    The `variable` operation produces an SSA value equal to some value
+    specified by an attribute. This can be used to form simple integer and
+    floating point variables, as well as more exotic things like tensor
+    variables. The `variable` operation also supports the EmitC opaque
+    attribute and the EmitC opaque type.
+    The `variable` is emitted as a C/C++ local variable.
+
+    Example:
+
+    ```mlir
+    // Integer variable
+    %0 = "emitc.variable"(){value = 42 : i32} : () -> i32
+
+    // Variable emitted as `int32_t* = NULL;`
+    %1 = "emitc.variable"()
+        {value = #emitc.opaque<"NULL"> : !emitc.opaque<"int32_t*">}
+        : () -> !emitc.opaque<"int32_t*">
+    ```
+  }];
+
+  let arguments = (ins AnyAttr:$value);
+  let results = (outs AnyType);
+
+  let hasVerifier = 1;
+}
+
 #endif // MLIR_DIALECT_EMITC_IR_EMITC

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

ParseResult IncludeOp::parse(OpAsmParser &parser, OperationState &result) {
   if (standardInclude)
     result.addAttribute("is_standard_include",
                         UnitAttr::get(parser.getContext()));
 
   return success();
 }
 
 //===----------------------------------------------------------------------===//
+// VariableOp
+//===----------------------------------------------------------------------===//
+
+/// The variable op requires that the attribute's type matches the return type.
+LogicalResult emitc::VariableOp::verify() {
+  Attribute value = valueAttr();
+  Type type = getType();
+  if (!value.getType().isa<NoneType>() && type != value.getType())
+    return emitOpError() << "requires attribute's type (" << value.getType()
+                         << ") to match op's return type (" << type << ")";
+  return success();
+}
+
+//===----------------------------------------------------------------------===//
 // TableGen'd op method definitions
 //===----------------------------------------------------------------------===//
 
 #define GET_OP_CLASSES
 #include "mlir/Dialect/EmitC/IR/EmitC.cpp.inc"
 
 //===----------------------------------------------------------------------===//
 // EmitC Attributes

mlir/lib/Target/Cpp/TranslateToCpp.cpp

static LogicalResult printOperation(CppEmitter &emitter,
                                     emitc::ConstantOp constantOp) {
   Operation *operation = constantOp.getOperation();
   Attribute value = constantOp.value();
 
   return printConstantOp(emitter, operation, value);
 }
 
 static LogicalResult printOperation(CppEmitter &emitter,
+                                    emitc::VariableOp variableOp) {
+  Operation *operation = variableOp.getOperation();
+  Attribute value = variableOp.value();
+
+  return printConstantOp(emitter, operation, value);
+}
+
+static LogicalResult printOperation(CppEmitter &emitter,
                                     arith::ConstantOp constantOp) {
   Operation *operation = constantOp.getOperation();
   Attribute value = constantOp.getValue();
 
   return printConstantOp(emitter, operation, value);
 }
 
 static LogicalResult printOperation(CppEmitter &emitter,
LogicalResult CppEmitter::emitLabel(Block &block) {
   return success();
 }
 
 LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) {
   LogicalResult status =
       llvm::TypeSwitch<Operation *, LogicalResult>(&op)
           // EmitC ops.
           .Case<emitc::ApplyOp, emitc::CallOp, emitc::ConstantOp,
-                emitc::IncludeOp>(
+                emitc::IncludeOp, emitc::VariableOp>(
               [&](auto op) { return printOperation(*this, op); })
           // SCF ops.
           .Case<scf::ForOp, scf::IfOp, scf::YieldOp>(
               [&](auto op) { return printOperation(*this, op); })
           // Standard ops.
           .Case<cf::BranchOp, mlir::CallOp, cf::CondBranchOp, mlir::ConstantOp,
                 FuncOp, ModuleOp, ReturnOp>(
               [&](auto op) { return printOperation(*this, op); })

mlir/test/Dialect/EmitC/invalid_ops.mlir

 // RUN: mlir-opt %s -split-input-file -verify-diagnostics
 
 func @const_attribute_return_type_1() {
     // expected-error @+1 {{'emitc.constant' op requires attribute's type ('i64') to match op's return type ('i32')}}
     %c0 = "emitc.constant"(){value = 42: i64} : () -> i32
     return
 }
 
 // -----
 
 func @const_attribute_return_type_2() {
-    // expected-error @+1 {{'emitc.constant' op requires attribute's type ('!emitc.opaque<"int32_t*">') to match op's return type ('!emitc.opaque<"int32_t">')}}
-    %c0 = "emitc.constant"(){value = "nullptr" : !emitc.opaque<"int32_t*">} : () -> !emitc.opaque<"int32_t">
+    // expected-error @+1 {{'emitc.constant' op requires attribute's type ('!emitc.opaque<"char">') to match op's return type ('!emitc.opaque<"mychar">')}}
+    %c0 = "emitc.constant"(){value = "CHAR_MIN" : !emitc.opaque<"char">} : () -> !emitc.opaque<"mychar">
     return
 }
 
 // -----
 
 func @index_args_out_of_range_1() {
     // expected-error @+1 {{'emitc.call' op index argument is out of range}}
     emitc.call "test" () {args = [0 : index]} : () -> ()

 
 // -----
 
 func @illegal_operator(%arg : i32) {
     // expected-error @+1 {{'emitc.apply' op applicable operator is illegal}}
     %2 = emitc.apply "+"(%arg) : (i32) -> !emitc.opaque<"int32_t*">
     return
 }
+
+// -----
+
+func @var_attribute_return_type_1() {
+    // expected-error @+1 {{'emitc.variable' op requires attribute's type ('i64') to match op's return type ('i32')}}
+    %c0 = "emitc.variable"(){value = 42: i64} : () -> i32
+    return
+}
+
+// -----
+
+func @var_attribute_return_type_2() {
+    // expected-error @+1 {{'emitc.variable' op requires attribute's type ('!emitc.opaque<"int32_t*">') to match op's return type ('!emitc.opaque<"int32_t">')}}
+    %c0 = "emitc.variable"(){value = "nullptr" : !emitc.opaque<"int32_t*">} : () -> !emitc.opaque<"int32_t">
+    return
+}

mlir/test/Dialect/EmitC/ops.mlir

 // RUN: mlir-opt %s | mlir-opt | FileCheck %s
 
 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">) {
   %1 = "emitc.call"() {callee = "blah"} : () -> i64
   emitc.call "foo" (%1) {args = [
     0 : index, dense<[0, 1]> : tensor<2xi32>, 0 : index
   ]} : (i64) -> ()
   return
 }
 
-func @c(%arg0: i32) {
+func @c() {
   %1 = "emitc.constant"(){value = 42 : i32} : () -> i32
   return
 }
 
 func @a(%arg0: i32, %arg1: i32) {
   %1 = "emitc.apply"(%arg0) {applicableOperator = "&"} : (i32) -> !emitc.opaque<"int32_t*">
   %2 = emitc.apply "&"(%arg1) : (i32) -> !emitc.opaque<"int32_t*">
   return
 }
+
+func @v() {
+  %1 = "emitc.variable"(){value = 21 : i32} : () -> i32
+  return
+}

mlir/test/Target/Cpp/const.mlir

 // RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s -check-prefix=CPP-DEFAULT
 // RUN: mlir-translate -mlir-to-cpp -declare-variables-at-top %s | FileCheck %s -check-prefix=CPP-DECLTOP
 
-
 func @emitc_constant() {
   %c0 = "emitc.constant"(){value = #emitc.opaque<""> : i32} : () -> i32
   %c1 = "emitc.constant"(){value = 42 : i32} : () -> i32
   %c2 = "emitc.constant"(){value = -1 : i32} : () -> i32
   %c3 = "emitc.constant"(){value = -1 : si8} : () -> si8
   %c4 = "emitc.constant"(){value = 255 : ui8} : () -> ui8
-  %c5 = "emitc.constant"(){value = #emitc.opaque<""> : !emitc.opaque<"int32_t*">} : () -> !emitc.opaque<"int32_t*">
-  %c6 = "emitc.constant"(){value = #emitc.opaque<"NULL"> : !emitc.opaque<"int32_t*">} : () -> !emitc.opaque<"int32_t*">
+  %c5 = "emitc.constant"(){value = #emitc.opaque<"CHAR_MIN"> : !emitc.opaque<"char">} : () -> !emitc.opaque<"char">
   return
 }
 // CPP-DEFAULT: void emitc_constant() {
 // CPP-DEFAULT-NEXT: int32_t [[V0:[^ ]*]];
 // CPP-DEFAULT-NEXT: int32_t [[V1:[^ ]*]] = 42;
 // CPP-DEFAULT-NEXT: int32_t [[V2:[^ ]*]] = -1;
 // CPP-DEFAULT-NEXT: int8_t [[V3:[^ ]*]] = -1;
 // CPP-DEFAULT-NEXT: uint8_t [[V4:[^ ]*]] = 255;
-// CPP-DEFAULT-NEXT: int32_t* [[V5:[^ ]*]];
-// CPP-DEFAULT-NEXT: int32_t* [[V6:[^ ]*]] = NULL;
+// CPP-DEFAULT-NEXT: char [[V5:[^ ]*]] = CHAR_MIN;
 
 // CPP-DECLTOP: void emitc_constant() {
 // CPP-DECLTOP-NEXT: int32_t [[V0:[^ ]*]];
 // CPP-DECLTOP-NEXT: int32_t [[V1:[^ ]*]];
 // CPP-DECLTOP-NEXT: int32_t [[V2:[^ ]*]];
 // CPP-DECLTOP-NEXT: int8_t [[V3:[^ ]*]];
 // CPP-DECLTOP-NEXT: uint8_t [[V4:[^ ]*]];
-// CPP-DECLTOP-NEXT: int32_t* [[V5:[^ ]*]];
-// CPP-DECLTOP-NEXT: int32_t* [[V6:[^ ]*]];
+// CPP-DECLTOP-NEXT: char [[V5:[^ ]*]];
 // CPP-DECLTOP-NEXT: ;
 // CPP-DECLTOP-NEXT: [[V1]] = 42;
 // CPP-DECLTOP-NEXT: [[V2]] = -1;
 // CPP-DECLTOP-NEXT: [[V3]] = -1;
 // CPP-DECLTOP-NEXT: [[V4]] = 255;
-// CPP-DECLTOP-NEXT: ;
-// CPP-DECLTOP-NEXT: [[V6]] = NULL;
+// CPP-DECLTOP-NEXT: [[V5]] = CHAR_MIN;