Addressing comments

Author: jpienaar

mlir/include/mlir/IR/BuiltinLocationAttributes.td

@@ -90,9 +90,9 @@ def FileLineColRange : Builtin_LocationAttr<"FileLineColRange"> {
     ```
   }];
 
-  // Note: this only shows the parameters for which accessors are generated. The
-  // locations are only set in storage.
-  let parameters = (ins "StringAttr":$filename);
+  let parameters = (ins "StringAttr":$filename,
+    "unsigned":$start_line, "unsigned":$start_column,
+    "unsigned":$end_line, "unsigned":$end_column);
   let builders = [
     AttrBuilderWithInferredContext<(ins "StringAttr":$filename), [{
       return $_get(filename.getContext(), filename, ArrayRef<unsigned>{});
@@ -142,12 +142,14 @@ def FileLineColRange : Builtin_LocationAttr<"FileLineColRange"> {
   ];
 
   let extraClassDeclaration = [{
+    ::mlir::StringAttr getFilename() const;
     std::optional<unsigned> getStartLine() const;
     std::optional<unsigned> getStartColumn() const;
     std::optional<unsigned> getEndColumn() const;
     std::optional<unsigned> getEndLine() const;
   }];
   let skipDefaultBuilders = 1;
+  let genAccessors = 0;
   let genStorageClass = 0;
   let attrName = "builtin.file_line_range";
 }

mlir/lib/IR/BuiltinDialectBytecode.cpp

@@ -73,8 +73,8 @@ readPotentiallySplatString(DialectBytecodeReader &reader, ShapedType type,
   return success();
 }
 
-void writePotentiallySplatString(DialectBytecodeWriter &writer,
-                                 DenseStringElementsAttr attr) {
+static void writePotentiallySplatString(DialectBytecodeWriter &writer,
+                                        DenseStringElementsAttr attr) {
   bool isSplat = attr.isSplat();
   if (isSplat)
     return writer.writeOwnedString(attr.getRawStringData().front());
@@ -83,8 +83,9 @@ void writePotentiallySplatString(DialectBytecodeWriter &writer,
     writer.writeOwnedString(str);
 }
 
-FileLineColRange getFileLineColRange(MLIRContext *context, StringAttr filename,
-                                     ArrayRef<uint64_t> lineCols) {
+static FileLineColRange getFileLineColRange(MLIRContext *context,
+                                            StringAttr filename,
+                                            ArrayRef<uint64_t> lineCols) {
   switch (lineCols.size()) {
   case 0:
     return FileLineColRange::get(filename);
@@ -97,34 +98,52 @@ FileLineColRange getFileLineColRange(MLIRContext *context, StringAttr filename,
                                  lineCols[2]);
   case 4:
     return FileLineColRange::get(filename, lineCols[0], lineCols[1],
-                                 lineCols[3], lineCols[2]);
+                                 lineCols[2], lineCols[3]);
   default:
     return {};
   }
 }
 
-LogicalResult readFileLineColRangeLocs(DialectBytecodeReader &reader,
+static LogicalResult readFileLineColRangeLocs(DialectBytecodeReader &reader,
                                        SmallVectorImpl<uint64_t> &lineCols) {
   return reader.readList(
       lineCols, [&reader](uint64_t &val) { return reader.readVarInt(val); });
 }
 
-void writeFileLineColRangeLocs(DialectBytecodeWriter &writer,
-                               FileLineColRange range) {
-  int count = range.getStartLine().has_value() +
-              range.getStartColumn().has_value() +
-              range.getEndLine().has_value() + range.getEndColumn().has_value();
-  writer.writeVarInt(count);
-  if (count == 0)
+static void writeFileLineColRangeLocs(DialectBytecodeWriter &writer,
+                                      FileLineColRange range) {
+  if (range.getStartLine() == range.getStartColumn() == range.getEndLine() ==
+      range.getEndColumn() == 0) {
+    writer.writeVarInt(0);
     return;
-  // Startline here is either known or zero with other trailing offsets.
-  writer.writeVarInt(range.getStartLine().value_or(0));
-  if (range.getStartColumn().has_value())
-    writer.writeVarInt(*range.getStartColumn());
-  if (range.getEndColumn().has_value())
-    writer.writeVarInt(*range.getEndColumn());
-  if (range.getEndLine().has_value())
-    writer.writeVarInt(*range.getEndLine());
+  }
+  if (range.getStartColumn() == 0 &&
+      range.getStartLine() == range.getEndLine()) {
+    writer.writeVarInt(1);
+    writer.writeVarInt(range.getStartLine());
+    return;
+  }
+  // The single file:line:col is handled by other writer, but checked here for
+  // completeness.
+  if (range.endColumn() == range.startColumn() &&
+      range.getStartLine() == range.getEndLine()) {
+    writer.writeVarInt(2);
+    writer.writeVarInt(range.getStartLine());
+    writer.writeVarInt(range.getStartColumn());
+    return;
+  }
+  if (range.getStartLine() == range.getEndLine()) {
+    writer.writeVarInt(3);
+    writer.writeVarInt(range.getStartLine());
+    writer.writeVarInt(range.getStartColumn());
+    writer.writeVarInt(range.getEndColumn());
+    return;
+  }
+  writer.writeVarInt(4);
+  writer.writeVarInt(range.getStartLine());
+  writer.writeVarInt(range.getStartColumn());
+  writer.writeVarInt(range.getEndLine());
+  writer.writeVarInt(range.getEndColumn());
 }
 
 #include "mlir/IR/BuiltinDialectBytecode.cpp.inc"

mlir/lib/IR/Location.cpp

@@ -78,21 +78,25 @@ struct FileLineColRangeAttrStorage final
             ArrayRef<unsigned>{std::get<1>(tblgenKey)}.drop_front());
   }
 
-  std::optional<unsigned> getLineCols(unsigned index) const {
-    if (size() <= index)
-      return std::nullopt;
+  unsigned getLineCols(unsigned index) const {
     return getTrailingObjects<unsigned>()[index - 1];
   }
 
-  std::optional<unsigned> getStartLine() const {
-    // Only return nullopt if there are no other locations and start line is 0.
-    if (startLine == 0 && filenameAndTrailing.getInt() == 0)
-      return std::nullopt;
+  unsigned getStartLine() const {
     return startLine;
   }
-  std::optional<unsigned> getStartColumn() const { return getLineCols(1); }
-  std::optional<unsigned> getEndColumn() const { return getLineCols(2); }
-  std::optional<unsigned> getEndLine() const { return getLineCols(3); }
+  unsigned getStartColumn() const {
+    if (size() <= 1) return 0;
+    return getLineCols(1);
+  }
+  unsigned getEndColumn() const {
+    if (size() <= 2) return getStartColumn();
+    return getLineCols(2);
+  }
+  unsigned getEndLine() const {
+    if (size() <= 3) return getStartLine();
+    return getLineCols(3);
+  }
 
   static ::llvm::hash_code hashKey(const KeyTy &tblgenKey) {
     return ::llvm::hash_combine(std::get<0>(tblgenKey), std::get<1>(tblgenKey));
@@ -188,16 +192,16 @@ StringAttr FileLineColRange::getFilename() const {
   return getImpl()->filenameAndTrailing.getPointer();
 }
 
-std::optional<unsigned> FileLineColRange::getStartLine() const {
+unsigned FileLineColRange::getStartLine() const {
   return getImpl()->getStartLine();
 }
-std::optional<unsigned> FileLineColRange::getStartColumn() const {
+unsigned FileLineColRange::getStartColumn() const {
   return getImpl()->getStartColumn();
 }
-std::optional<unsigned> FileLineColRange::getEndColumn() const {
+unsigned FileLineColRange::getEndColumn() const {
   return getImpl()->getEndColumn();
 }
-std::optional<unsigned> FileLineColRange::getEndLine() const {
+unsigned FileLineColRange::getEndLine() const {
   return getImpl()->getEndLine();
 }