From 12157a821a4e261619b54008834dfbb1034ba829 Mon Sep 17 00:00:00 2001 From: Luis Miguel Sousa Date: Tue, 26 Nov 2024 15:12:57 +0000 Subject: [PATCH 1/4] Include comments with template arg names in Cpp code from EmitC --- mlir/include/mlir/Dialect/EmitC/IR/EmitC.td | 3 ++- mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 8 ++++++ mlir/lib/Target/Cpp/TranslateToCpp.cpp | 26 +++++++++++++++++--- mlir/test/Dialect/EmitC/invalid_ops.mlir | 16 ++++++++++++ mlir/test/Dialect/EmitC/ops.mlir | 7 ++++++ mlir/test/Target/Cpp/template_arg_names.mlir | 20 +++++++++++++++ 6 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 mlir/test/Target/Cpp/template_arg_names.mlir diff --git a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td index 0015ff0cd8810..681c8709e574b 100644 --- a/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td +++ b/mlir/include/mlir/Dialect/EmitC/IR/EmitC.td @@ -292,6 +292,7 @@ def EmitC_CallOpaqueOp : EmitC_Op<"call_opaque", [CExpression]> { Arg:$callee, Arg, "the order of operands and further attributes">:$args, Arg, "template arguments">:$template_args, + Arg, "template argument names">:$template_arg_names, Variadic:$operands ); let results = (outs Variadic); @@ -302,7 +303,7 @@ def EmitC_CallOpaqueOp : EmitC_Op<"call_opaque", [CExpression]> { "::mlir::ValueRange":$operands, CArg<"::mlir::ArrayAttr", "{}">:$args, CArg<"::mlir::ArrayAttr", "{}">:$template_args), [{ - build($_builder, $_state, resultTypes, callee, args, template_args, + build($_builder, $_state, resultTypes, callee, args, template_args, {}, operands); }] > diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp index c2fb835c2ebc3..612575cffd55a 100644 --- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp +++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp @@ -353,6 +353,14 @@ LogicalResult emitc::CallOpaqueOp::verify() { if (!llvm::isa(tArg)) return emitOpError("template argument has invalid type"); } + + if (std::optional templateArgNames = getTemplateArgNames()) { + if ((*templateArgNames).size() && + (*templateArgNames).size() != (*templateArgsAttr).size()) { + return emitOpError("number of template argument names must be equal to " + "number of template arguments"); + } + } } if (llvm::any_of(getResultTypes(), llvm::IsaPred)) { diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp index cc32828a0c544..fe17afa7df5cd 100644 --- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp +++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp @@ -659,11 +659,31 @@ static LogicalResult printOperation(CppEmitter &emitter, return success(); }; + auto emitNamedArgs = + [&](std::tuple tuple) + -> LogicalResult { + Attribute attr = std::get<0>(tuple); + StringAttr argName = cast(std::get<1>(tuple)); + + os << "/*" << argName.str() << "=*/"; + return emitArgs(attr); + }; + if (callOpaqueOp.getTemplateArgs()) { os << "<"; - if (failed(interleaveCommaWithError(*callOpaqueOp.getTemplateArgs(), os, - emitArgs))) - return failure(); + if (callOpaqueOp.getTemplateArgNames() && + !callOpaqueOp.getTemplateArgNames()->empty()) { + if (failed(interleaveCommaWithError( + llvm::zip(*callOpaqueOp.getTemplateArgs(), + *callOpaqueOp.getTemplateArgNames()), + os, emitNamedArgs))) { + return failure(); + } + } else { + if (failed(interleaveCommaWithError(*callOpaqueOp.getTemplateArgs(), os, + emitArgs))) + return failure(); + } os << ">"; } diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir index 3b4c6046a08c5..44fd7bb263992 100644 --- a/mlir/test/Dialect/EmitC/invalid_ops.mlir +++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir @@ -524,3 +524,19 @@ func.func @test_verbatim(%arg0 : !emitc.ptr, %arg1 : i32) { emitc.verbatim "{a} " args %arg0, %arg1 : !emitc.ptr, i32 return } + +// ----- + +func.func @template_args_with_names(%arg0: i32) { + // expected-error @+1 {{'emitc.call_opaque' op number of template argument names must be equal to number of template arguments}} + emitc.call_opaque "kernel1"(%arg0) {template_arg_names = ["N", "P"], template_args = [42 : i32]} : (i32) -> () + return +} + +// ----- + +func.func @template_args_with_names2(%arg0: i32) { + // expected-error @+1 {{'emitc.call_opaque' op number of template argument names must be equal to number of template arguments}} + emitc.call_opaque "kernel1"(%arg0) {template_arg_names = ["N"], template_args = [42 : i32, 56 : i32]} : (i32) -> () + return +} diff --git a/mlir/test/Dialect/EmitC/ops.mlir b/mlir/test/Dialect/EmitC/ops.mlir index 4e86642c2a3a9..8fe0a828c84db 100644 --- a/mlir/test/Dialect/EmitC/ops.mlir +++ b/mlir/test/Dialect/EmitC/ops.mlir @@ -282,3 +282,10 @@ func.func @member_access(%arg0: !emitc.opaque<"mystruct">, %arg1: !emitc.opaque< %2 = "emitc.member_of_ptr" (%arg2) {member = "a"} : (!emitc.ptr>) -> i32 return } + +func.func @template_args_with_names(%arg0: i32, %arg1: f32) { + emitc.call_opaque "kernel1"(%arg0, %arg1) {template_arg_names = ["N", "P"], template_args = [42 : i32, 56]} : (i32, f32) -> () + emitc.call_opaque "kernel2"(%arg0, %arg1) {template_arg_names = ["N"], template_args = [42 : i32]} : (i32, f32) -> () + emitc.call_opaque "kernel3"(%arg0, %arg1) {template_arg_names = [], template_args = [#emitc.opaque<"42">]} : (i32, f32) -> () + return +} diff --git a/mlir/test/Target/Cpp/template_arg_names.mlir b/mlir/test/Target/Cpp/template_arg_names.mlir new file mode 100644 index 0000000000000..d0a6ee77986a2 --- /dev/null +++ b/mlir/test/Target/Cpp/template_arg_names.mlir @@ -0,0 +1,20 @@ +// RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s -check-prefix=CPP-DEFAULT + +// CPP-DEFAULT: void basic(int32_t v1, float v2) { +func.func @basic(%arg0: i32, %arg1: f32) { + emitc.call_opaque "kernel1"() : () -> () +// CPP-DEFAULT: kernel1(); + %0 = emitc.call_opaque "kernel2"(%arg0) : (i32) -> i16 +// CPP-DEFAULT: int16_t v3 = kernel2(v1); + emitc.call_opaque "kernel3"(%arg0, %arg1) : (i32, f32) -> () +// CPP-DEFAULT: kernel3(v1, v2); + emitc.call_opaque "kernel4"(%arg0, %arg1) {template_arg_names = ["N", "P"], template_args = [42 : i32, 56]} : (i32, f32) -> () +// CPP-DEFAULT: kernel4(v1, v2); + emitc.call_opaque "kernel4"(%arg0, %arg1) {template_arg_names = ["N"], template_args = [#emitc.opaque<"42">]} : (i32, f32) -> () +// CPP-DEFAULT: kernel4(v1, v2); + return +// CPP-DEFAULT: return; +} +// CPP-DEFAULT: } + + From bbebd27fb6073a472dd4b9074c9e88180401d18f Mon Sep 17 00:00:00 2001 From: lmendesp-amd Date: Tue, 26 Nov 2024 17:00:24 +0100 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Corentin Ferry Co-authored-by: Matthias Gehre --- mlir/lib/Target/Cpp/TranslateToCpp.cpp | 2 +- mlir/test/Target/Cpp/template_arg_names.mlir | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp index fe17afa7df5cd..b4d6660509675 100644 --- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp +++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp @@ -674,7 +674,7 @@ static LogicalResult printOperation(CppEmitter &emitter, if (callOpaqueOp.getTemplateArgNames() && !callOpaqueOp.getTemplateArgNames()->empty()) { if (failed(interleaveCommaWithError( - llvm::zip(*callOpaqueOp.getTemplateArgs(), + llvm::zip_equal(*callOpaqueOp.getTemplateArgs(), *callOpaqueOp.getTemplateArgNames()), os, emitNamedArgs))) { return failure(); diff --git a/mlir/test/Target/Cpp/template_arg_names.mlir b/mlir/test/Target/Cpp/template_arg_names.mlir index d0a6ee77986a2..113bcc9e5a643 100644 --- a/mlir/test/Target/Cpp/template_arg_names.mlir +++ b/mlir/test/Target/Cpp/template_arg_names.mlir @@ -1,20 +1,14 @@ // RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s -check-prefix=CPP-DEFAULT -// CPP-DEFAULT: void basic(int32_t v1, float v2) { +// CPP-DEFAULT-LABEL: void basic func.func @basic(%arg0: i32, %arg1: f32) { - emitc.call_opaque "kernel1"() : () -> () -// CPP-DEFAULT: kernel1(); - %0 = emitc.call_opaque "kernel2"(%arg0) : (i32) -> i16 -// CPP-DEFAULT: int16_t v3 = kernel2(v1); emitc.call_opaque "kernel3"(%arg0, %arg1) : (i32, f32) -> () // CPP-DEFAULT: kernel3(v1, v2); emitc.call_opaque "kernel4"(%arg0, %arg1) {template_arg_names = ["N", "P"], template_args = [42 : i32, 56]} : (i32, f32) -> () // CPP-DEFAULT: kernel4(v1, v2); emitc.call_opaque "kernel4"(%arg0, %arg1) {template_arg_names = ["N"], template_args = [#emitc.opaque<"42">]} : (i32, f32) -> () -// CPP-DEFAULT: kernel4(v1, v2); +// CPP-DEFAULT: kernel4( return -// CPP-DEFAULT: return; } -// CPP-DEFAULT: } From edd8cc3057e0ecfc78784bb103214ed611a58ab3 Mon Sep 17 00:00:00 2001 From: Luis Miguel Sousa Date: Tue, 26 Nov 2024 16:05:22 +0000 Subject: [PATCH 3/4] Apply changes requested in code review --- mlir/lib/Target/Cpp/TranslateToCpp.cpp | 2 +- mlir/test/Target/Cpp/template_arg_names.mlir | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp index b4d6660509675..ee94967ab56a8 100644 --- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp +++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp @@ -675,7 +675,7 @@ static LogicalResult printOperation(CppEmitter &emitter, !callOpaqueOp.getTemplateArgNames()->empty()) { if (failed(interleaveCommaWithError( llvm::zip_equal(*callOpaqueOp.getTemplateArgs(), - *callOpaqueOp.getTemplateArgNames()), + *callOpaqueOp.getTemplateArgNames()), os, emitNamedArgs))) { return failure(); } diff --git a/mlir/test/Target/Cpp/template_arg_names.mlir b/mlir/test/Target/Cpp/template_arg_names.mlir index 113bcc9e5a643..f4e504b059474 100644 --- a/mlir/test/Target/Cpp/template_arg_names.mlir +++ b/mlir/test/Target/Cpp/template_arg_names.mlir @@ -3,9 +3,9 @@ // CPP-DEFAULT-LABEL: void basic func.func @basic(%arg0: i32, %arg1: f32) { emitc.call_opaque "kernel3"(%arg0, %arg1) : (i32, f32) -> () -// CPP-DEFAULT: kernel3(v1, v2); +// CPP-DEFAULT: kernel3( emitc.call_opaque "kernel4"(%arg0, %arg1) {template_arg_names = ["N", "P"], template_args = [42 : i32, 56]} : (i32, f32) -> () -// CPP-DEFAULT: kernel4(v1, v2); +// CPP-DEFAULT: kernel4( emitc.call_opaque "kernel4"(%arg0, %arg1) {template_arg_names = ["N"], template_args = [#emitc.opaque<"42">]} : (i32, f32) -> () // CPP-DEFAULT: kernel4( return From 25c3d047110b19cc8cdba06a8a36898362d9c73a Mon Sep 17 00:00:00 2001 From: Luis Miguel Sousa Date: Tue, 26 Nov 2024 16:15:12 +0000 Subject: [PATCH 4/4] Test for the presence of template arg names when there are no template args --- mlir/lib/Dialect/EmitC/IR/EmitC.cpp | 7 ++++++- mlir/test/Dialect/EmitC/invalid_ops.mlir | 10 +++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp index 612575cffd55a..7c8267a234368 100644 --- a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp +++ b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp @@ -353,13 +353,18 @@ LogicalResult emitc::CallOpaqueOp::verify() { if (!llvm::isa(tArg)) return emitOpError("template argument has invalid type"); } + } - if (std::optional templateArgNames = getTemplateArgNames()) { + if (std::optional templateArgNames = getTemplateArgNames()) { + if (std::optional templateArgsAttr = getTemplateArgs()) { if ((*templateArgNames).size() && (*templateArgNames).size() != (*templateArgsAttr).size()) { return emitOpError("number of template argument names must be equal to " "number of template arguments"); } + } else { + return emitOpError("should not have names for template arguments if it " + "does not have template arguments"); } } diff --git a/mlir/test/Dialect/EmitC/invalid_ops.mlir b/mlir/test/Dialect/EmitC/invalid_ops.mlir index 44fd7bb263992..7f0e89b57b01a 100644 --- a/mlir/test/Dialect/EmitC/invalid_ops.mlir +++ b/mlir/test/Dialect/EmitC/invalid_ops.mlir @@ -535,8 +535,16 @@ func.func @template_args_with_names(%arg0: i32) { // ----- -func.func @template_args_with_names2(%arg0: i32) { +func.func @template_args_with_names(%arg0: i32) { // expected-error @+1 {{'emitc.call_opaque' op number of template argument names must be equal to number of template arguments}} emitc.call_opaque "kernel1"(%arg0) {template_arg_names = ["N"], template_args = [42 : i32, 56 : i32]} : (i32) -> () return } + +// ----- + +func.func @template_args_with_names(%arg0: i32) { + // expected-error @+1 {{'emitc.call_opaque' op should not have names for template arguments if it does not have template arguments}} + emitc.call_opaque "kernel1"(%arg0) {template_arg_names = ["N"]} : (i32) -> () + return +}