[PATCH] D76326: Introduced CallOp Dialect Conversion

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Looks good, just have a question about the location.



================
Comment at: mlir/include/mlir/Dialect/StandardOps/Transforms/DialectConversion.h:1
+//===- DialectConversion.h - MLIR dialect conversion pass -------*- C++ -*-===//
+//
----------------
Would this make more sense in the Conversion directory? Say `mlir/Conversion/StandardToStandard`?


================
Comment at: mlir/include/mlir/Dialect/StandardOps/Transforms/DialectConversion.h:9
+//
+// This file declares a generic pass for converting between MLIR dialects
+// related to the StandardOps Dialect.
----------------
This seems like it should be updated, likely should just mention that it contains patterns for lowering within standard dialect.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/Transforms/DialectConversion.h:17
+
+#include "mlir/IR/PatternMatch.h"
+#include "mlir/Support/LLVM.h"
----------------
I think we can trim all of these and just use forward declarations.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/Transforms/DialectConversion.h:29
+
+/// Add a pattern to the given pattern list to convert the result types of a
+/// CallOp with the given type converter.
----------------
operand and result types?


================
Comment at: mlir/lib/Dialect/StandardOps/Transforms/DialectConversion.cpp:1
+
+#include "mlir/Dialect/StandardOps/Transforms/DialectConversion.h"
----------------
Missing a file header here.


================
Comment at: mlir/lib/Dialect/StandardOps/Transforms/DialectConversion.cpp:6
+
+namespace mlir {
+
----------------
nit: prefer `using namespace mlir` in .cpp files


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76326/new/

https://reviews.llvm.org/D76326