[PATCH] D73944: [mlir][wip] Start Shape dialect

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:3
+//
+// Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
MLIR -> LLVM


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:9
+//
+// This file defines the shape dialect that is used to describe & solve shape
+// relantions of MLIR operations using ShapedType.
----------------
nit: & -> and


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:10
+// This file defines the shape dialect that is used to describe & solve shape
+// relantions of MLIR operations using ShapedType.
+//
----------------
typo: relations?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:51
+
+  // Support method to enable LLVM-style type casting.
+  static bool kindof(unsigned kind) { return kind == ShapeTypes::Kind::Dim; }
----------------
///?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:55
+
+// The shape descriptor type represents rank and dimension sizes.
+class ShapeType : public Type::TypeBase<ShapeType, Type> {
----------------
///


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:68
+
+class ElementType : public Type::TypeBase<ElementType, Type> {
+public:
----------------
Can we get comments on each of these types?


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:96
+
+// The ValueShape represents a (potentially unknown) runtime value and shape.
+class ValueShapeType : public Type::TypeBase<ValueShapeType, Type> {
----------------
///


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:115
+} // namespace shape
+}  // namespace mlir
+
----------------
weird spacing here.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/Shape.h:117
+
+#endif // MLIR_SHAPE_SHAPE_H
----------------
This header comment looks off.


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:3
+//
+// Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
MLIR -> LLVM


================
Comment at: mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td:71
+
+def Shape_ElementType : DialectType<ShapeDialect, CPred<"$_self.isa<ElementType>()">, "element type"> {
+  let typeDescription = [{
----------------
Are these wrapped at 80 characters? They look like they are overflowing.


================
Comment at: mlir/include/mlir/IR/DialectSymbolRegistry.def:27
 DEFINE_SYM_KIND_RANGE(XLA_HLO) // XLA HLO dialect
+DEFINE_SYM_KIND_RANGE(SHAPE) // Shape dialect
 
----------------
Not this revision, but we should sort these.


================
Comment at: mlir/lib/Dialect/Shape/DialectRegistration.cpp:3
+//
+// Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
MLIR -> LLVM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73944