[TableGen] Rework error reporting for duplicate Feature/Processor (#102257)

- Extract code for sorting and checking duplicate Records into a helper
  function and update `collectProcModels` to use the helper.
- Update `FeatureKeyValues` to:
  (a) Remove code for duplicate checks and use the helper.
  (b) Trim features with empty name explicitly to be able to use
      the helper.
- Make the sorting deterministic by using record name as a secondary
  key for sorting, and re-enable SubtargetFeatureUniqueNames.td test
  that was disabled due to the non-determinism of the error messages.
- Change wording of error message when duplicate records are found to
  be source code position agnostic, since `First` may not be before
  `Second` lexically.

llvm/include/llvm/TableGen/Record.h

@@ -2112,8 +2112,7 @@ struct LessRecordByID {
   }
 };
 
-/// Sorting predicate to sort record pointers by their
-/// name field.
+/// Sorting predicate to sort record pointers by their Name field.
 struct LessRecordFieldName {
   bool operator()(const Record *Rec1, const Record *Rec2) const {
     return Rec1->getValueAsString("Name") < Rec2->getValueAsString("Name");

llvm/test/TableGen/ProcessorUniqueNames.td

@@ -0,0 +1,12 @@
+// RUN: not llvm-tblgen -gen-subtarget -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
+// Verify that processors with same names result in an error.
+
+include "llvm/Target/Target.td"
+
+def MyTarget : Target;
+
+def ProcessorB : ProcessorModel<"NameA", NoSchedModel, []>;
+
+// CHECK: [[FILE]]:[[@LINE+2]]:5: error: Processor `NameA` is already defined.
+// CHECK: [[FILE]]:[[@LINE-3]]:5: note: Previous definition here.
+def ProcessorA : ProcessorModel<"NameA", NoSchedModel, []>;

llvm/test/TableGen/SubtargetFeatureUniqueNames.td

@@ -1,6 +1,3 @@
-// Temporarily disable test due to non-deterministic order of error messages.
-// UNSUPPORTED: target={{.*}}
-
 // RUN: not llvm-tblgen -gen-subtarget -I %p/../../include %s 2>&1 | FileCheck %s -DFILE=%s
 // Verify that subtarget features with same names result in an error.
 
@@ -8,8 +5,8 @@ include "llvm/Target/Target.td"
 
 def MyTarget : Target;
 
-def FeatureA : SubtargetFeature<"NameA", "", "", "">;
-
-// CHECK: [[FILE]]:[[@LINE+2]]:5: error: Feature `NameA` already defined.
-// CHECK: [[FILE]]:[[@LINE-3]]:5: note: Previous definition here.
 def FeatureB : SubtargetFeature<"NameA", "", "", "">;
+
+// CHECK: [[FILE]]:[[@LINE+2]]:5: error: Feature `NameA` is already defined.
+// CHECK: [[FILE]]:[[@LINE-3]]:5: note: Previous definition here.
+def FeatureA : SubtargetFeature<"NameA", "", "", "">;

llvm/utils/TableGen/Common/CMakeLists.txt

@@ -33,6 +33,7 @@ add_llvm_library(LLVMTableGenCommon STATIC OBJECT EXCLUDE_FROM_ALL
   PredicateExpander.cpp
   SubtargetFeatureInfo.cpp
   Types.cpp
+  Utils.cpp
   VarLenCodeEmitterGen.cpp
 
   LINK_LIBS

llvm/utils/TableGen/Common/CodeGenSchedule.cpp

@@ -14,6 +14,7 @@
 #include "CodeGenSchedule.h"
 #include "CodeGenInstruction.h"
 #include "CodeGenTarget.h"
+#include "Utils.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
@@ -533,17 +534,9 @@ void CodeGenSchedModels::collectOptionalProcessorInfo() {
 /// Gather all processor models.
 void CodeGenSchedModels::collectProcModels() {
   RecVec ProcRecords = Records.getAllDerivedDefinitions("Processor");
-  llvm::sort(ProcRecords, LessRecordFieldName());
 
-  // Check for duplicated names.
-  auto I = std::adjacent_find(ProcRecords.begin(), ProcRecords.end(),
-                              [](const Record *Rec1, const Record *Rec2) {
-                                return Rec1->getValueAsString("Name") ==
-                                       Rec2->getValueAsString("Name");
-                              });
-  if (I != ProcRecords.end())
-    PrintFatalError((*I)->getLoc(), "Duplicate processor name " +
-                                        (*I)->getValueAsString("Name"));
+  // Sort and check duplicate Processor name.
+  sortAndReportDuplicates(ProcRecords, "Processor");
 
   // Reserve space because we can. Reallocation would be ok.
   ProcModels.reserve(ProcRecords.size() + 1);

llvm/utils/TableGen/Common/Types.h

@@ -6,8 +6,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLVM_UTILS_TABLEGEN_TYPES_H
-#define LLVM_UTILS_TABLEGEN_TYPES_H
+#ifndef LLVM_UTILS_TABLEGEN_COMMON_TYPES_H
+#define LLVM_UTILS_TABLEGEN_COMMON_TYPES_H
 
 #include <cstdint>
 
@@ -18,4 +18,4 @@ namespace llvm {
 const char *getMinimalTypeForRange(uint64_t Range, unsigned MaxSize = 64);
 } // namespace llvm
 
-#endif
+#endif // LLVM_UTILS_TABLEGEN_COMMON_TYPES_H

llvm/utils/TableGen/Common/Utils.cpp

@@ -0,0 +1,48 @@
+//===- Utils.cpp - Common Utilities -----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Utils.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/TableGen/Error.h"
+#include "llvm/TableGen/Record.h"
+#include <algorithm>
+
+using namespace llvm;
+
+namespace {
+/// Sorting predicate to sort record pointers by their Name field, and break
+/// ties using record ID (which corresponds to creation/parse order).
+struct LessRecordFieldNameAndID {
+  bool operator()(const Record *Rec1, const Record *Rec2) const {
+    return std::tuple(Rec1->getValueAsString("Name"), Rec1->getID()) <
+           std::tuple(Rec2->getValueAsString("Name"), Rec2->getID());
+  }
+};
+} // End anonymous namespace
+
+/// Sort an array of Records on the "Name" field, and check for records with
+/// duplicate "Name" field. If duplicates are found, report a fatal error.
+void llvm::sortAndReportDuplicates(MutableArrayRef<Record *> Records,
+                                   StringRef ObjectName) {
+  llvm::sort(Records, LessRecordFieldNameAndID());
+
+  auto I = std::adjacent_find(Records.begin(), Records.end(),
+                              [](const Record *Rec1, const Record *Rec2) {
+                                return Rec1->getValueAsString("Name") ==
+                                       Rec2->getValueAsString("Name");
+                              });
+  if (I == Records.end())
+    return;
+
+  // Found a duplicate name.
+  const Record *First = *I;
+  const Record *Second = *(I + 1);
+  StringRef Name = First->getValueAsString("Name");
+  PrintError(Second, ObjectName + " `" + Name + "` is already defined.");
+  PrintFatalNote(First, "Previous definition here.");
+}

llvm/utils/TableGen/Common/Utils.h

@@ -0,0 +1,25 @@
+//===- Utils.h - Common Utilities -------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_UTILS_TABLEGEN_COMMON_UTILS_H
+#define LLVM_UTILS_TABLEGEN_COMMON_UTILS_H
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace llvm {
+class Record;
+
+/// Sort an array of Records on the "Name" field, and check for records with
+/// duplicate "Name" field. If duplicates are found, report a fatal error.
+void sortAndReportDuplicates(MutableArrayRef<Record *> Records,
+                             StringRef ObjectName);
+
+} // namespace llvm
+
+#endif // LLVM_UTILS_TABLEGEN_COMMON_UTILS_H

llvm/utils/TableGen/SubtargetEmitter.cpp

@@ -14,6 +14,7 @@
 #include "Common/CodeGenSchedule.h"
 #include "Common/CodeGenTarget.h"
 #include "Common/PredicateExpander.h"
+#include "Common/Utils.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
@@ -250,34 +251,30 @@ void SubtargetEmitter::EmitSubtargetInfoMacroCalls(raw_ostream &OS) {
 //
 unsigned SubtargetEmitter::FeatureKeyValues(
     raw_ostream &OS, const DenseMap<Record *, unsigned> &FeatureMap) {
-  // Gather and sort all the features
   std::vector<Record *> FeatureList =
       Records.getAllDerivedDefinitions("SubtargetFeature");
 
+  // Remove features with empty name.
+  llvm::erase_if(FeatureList, [](const Record *Rec) {
+    return Rec->getValueAsString("Name").empty();
+  });
   if (FeatureList.empty())
     return 0;
 
-  llvm::sort(FeatureList, LessRecordFieldName());
+  // Sort and check duplicate Feature name.
+  sortAndReportDuplicates(FeatureList, "Feature");
 
-  // Check that there are no duplicate features.
-  DenseMap<StringRef, const Record *> UniqueFeatures;
-
-  // Begin feature table
+  // Begin feature table.
   OS << "// Sorted (by key) array of values for CPU features.\n"
      << "extern const llvm::SubtargetFeatureKV " << Target
      << "FeatureKV[] = {\n";
 
-  // For each feature
-  unsigned NumFeatures = 0;
   for (const Record *Feature : FeatureList) {
     // Next feature
     StringRef Name = Feature->getName();
     StringRef CommandLineName = Feature->getValueAsString("Name");
     StringRef Desc = Feature->getValueAsString("Desc");
 
-    if (CommandLineName.empty())
-      continue;
-
     // Emit as { "feature", "description", { featureEnum }, { i1 , i2 , ... , in
     // } }
     OS << "  { "
@@ -289,20 +286,12 @@ unsigned SubtargetEmitter::FeatureKeyValues(
     printFeatureMask(OS, ImpliesList, FeatureMap);
 
     OS << " },\n";
-    ++NumFeatures;
-
-    auto [It, Inserted] = UniqueFeatures.insert({CommandLineName, Feature});
-    if (!Inserted) {
-      PrintError(Feature, "Feature `" + CommandLineName + "` already defined.");
-      const Record *Previous = It->second;
-      PrintFatalNote(Previous, "Previous definition here.");
-    }
   }
 
-  // End feature table
+  // End feature table.
   OS << "};\n";
 
-  return NumFeatures;
+  return FeatureList.size();
 }
 
 //
@@ -317,18 +306,22 @@ SubtargetEmitter::CPUKeyValues(raw_ostream &OS,
       Records.getAllDerivedDefinitions("Processor");
   llvm::sort(ProcessorList, LessRecordFieldName());
 
-  // Begin processor table
+  // Note that unlike `FeatureKeyValues`, here we do not need to check for
+  // duplicate processors, since that is already done when the SubtargetEmitter
+  // constructor calls `getSchedModels` to build a `CodeGenSchedModels` object,
+  // which does the duplicate processor check.
+
+  // Begin processor table.
   OS << "// Sorted (by key) array of values for CPU subtype.\n"
      << "extern const llvm::SubtargetSubTypeKV " << Target
      << "SubTypeKV[] = {\n";
 
-  // For each processor
   for (Record *Processor : ProcessorList) {
     StringRef Name = Processor->getValueAsString("Name");
     RecVec FeatureList = Processor->getValueAsListOfDefs("Features");
     RecVec TuneFeatureList = Processor->getValueAsListOfDefs("TuneFeatures");
 
-    // Emit as { "cpu", "description", 0, { f1 , f2 , ... fn } },
+    // Emit as "{ "cpu", "description", 0, { f1 , f2 , ... fn } },".
     OS << " { "
        << "\"" << Name << "\", ";
 
@@ -342,7 +335,7 @@ SubtargetEmitter::CPUKeyValues(raw_ostream &OS,
     OS << ", &" << ProcModelName << " },\n";
   }
 
-  // End processor table
+  // End processor table.
   OS << "};\n";
 
   return ProcessorList.size();