[mlir][ods] Rework how transitive use of deprecated defs are handled

[zero9178]

Summary

The code currently attempting to recursively find uses of a deprecated def has a few deficiences:

• It recurses into all def uses. This is problematic as it also causes any users of a def using a deprecated def, to be considered deprecated, causing a transitive chain of deprecated defs (see H_ButNotTransitivelyInNonAnonymousDef in test case for reproducer)
• It did not recurse into other kinds of fields, such as list and DAGs

This patch fixes the issue by reworking the code to properly recurse into inits and not to recurse into def uses unless they are anonymous defs. Since inits (including DAG, List and anonymous defs) are uniqued, the memoization is kept and remains profitable.

[jpienaar]

Nice, thanks

mlir/lib/Tools/mlir-tblgen/MlirTblgenMain.cpp

47–69

This is cheaper than the lookup, could we do this first?

mlir/test/mlir-tblgen/deprecation-transitive.td

10

Can we check no warning emitted here?

15

I can't think of a better way than CHECK-NOTs or running test 2x to verify that no other warnings were emitted. CHECK-NOT is reasonably simple luckily. This way we can check no unexpected warnings are emitted of slips in.

[zero9178]

Address review comments

• Move check for init value before the lookup
• Make use of --implicit-check-not warning: in FileCheck. This verifies that there are precisely no other warnings but the ones explicitly CHECKed

[zero9178]

mlir/test/mlir-tblgen/deprecation-transitive.td

15

I've addressed this and the above using --implicit-check-not warning: in FileCheck, which was perfect for this

mlir/lib/Tools/mlir-tblgen/MlirTblgenMain.cpp

 
 using namespace mlir;
 using namespace llvm;
 
 enum DeprecatedAction { None, Warn, Error };
 
 static DeprecatedAction actionOnDeprecatedValue;
 
-// Returns if there is a use of `init` in `record`.
-static bool findUse(Record &record, Init *init,
-                    llvm::DenseMap<Record *, bool> &known) {
-  auto it = known.find(&record);
+// Returns if there is a use of `deprecatedInit` in `field`.
+static bool findUse(Init *field, Init *deprecatedInit,
+                    llvm::DenseMap<Init *, bool> &known) {
+  if (field == deprecatedInit)
+    return true;
+
+  auto it = known.find(field);
   if (it != known.end())
     return it->second;
 
   auto memoize = [&](bool val) {
-    known[&record] = val;
+    known[field] = val;
     return val;
   };
 
-  for (const RecordVal &val : record.getValues()) {
-    Init *valInit = val.getValue();
-    if (valInit == init)
-      return true;
-    if (auto *di = dyn_cast<DefInit>(valInit)) {
-      if (findUse(*di->getDef(), init, known))
-        return memoize(true);
-    } else if (auto *di = dyn_cast<DagInit>(valInit)) {
-      for (Init *arg : di->getArgs())
-        if (auto *di = dyn_cast<DefInit>(arg))
-          if (findUse(*di->getDef(), init, known))
-            return memoize(true);
-    } else if (ListInit *li = dyn_cast<ListInit>(valInit)) {
-      for (Init *jt : li->getValues())
-        if (jt == init)
-          return memoize(true);
-    }
+  if (auto *defInit = dyn_cast<DefInit>(field)) {
+    // Only recurse into defs if they are anonymous.
+    // Non-anonymous defs are handled by the main loop, with a proper
+    // deprecation warning for each. Returning true here, would cause
+    // all users of a def to also emit a deprecation warning.
+    if (!defInit->getDef()->isAnonymous())
+      // Purposefully not memoize as to not include every def use in the map.
+      // This is also a trivial case we return false for in constant time.
+      return false;
+
+    return memoize(
+        llvm::any_of(defInit->getDef()->getValues(), [&](const RecordVal &val) {
+          return findUse(val.getValue(), deprecatedInit, known);
+        }));
+  }
+
+  if (auto *dagInit = dyn_cast<DagInit>(field)) {
+    if (findUse(dagInit->getOperator(), deprecatedInit, known))
+
+    return memoize(llvm::any_of(dagInit->getArgs(), [&](Init *arg) {
+      return findUse(arg, deprecatedInit, known);
+    }));
+
+  if (ListInit *li = dyn_cast<ListInit>(field)) {
+    return memoize(llvm::any_of(li->getValues(), [&](Init *jt) {
+      return findUse(jt, deprecatedInit, known);
+    }));
   }
-  return memoize(false);
+
+  // Purposefully don't use memoize here. There is no need to cache the result
+  // for every kind of init (e.g. BitInit or StringInit), which will always
+  // return false. Doing so would grow the DenseMap to include almost every Init
+  // within the main file.
+  return false;
+}
+
+// Returns if there is a use of `deprecatedInit` in `record`.
+static bool findUse(Record &record, Init *deprecatedInit,
+                    llvm::DenseMap<Init *, bool> &known) {
+  return llvm::any_of(record.getValues(), [&](const RecordVal &val) {
+    return findUse(val.getValue(), deprecatedInit, known);
+  });
 }
 
 static void warnOfDeprecatedUses(RecordKeeper &records) {
   // This performs a direct check for any def marked as deprecated and then
   // finds all uses of deprecated def. Deprecated defs are not expected to be
   // either numerous or long lived.
   bool deprecatedDefsFounds = false;
   for (auto &it : records.getDefs()) {
     const RecordVal *r = it.second->getValue("odsDeprecated");
     if (!r || !r->getValue())
       continue;
 
-    llvm::DenseMap<Record *, bool> hasUse;
+    llvm::DenseMap<Init *, bool> hasUse;
     if (auto *si = dyn_cast<StringInit>(r->getValue())) {
       for (auto &jt : records.getDefs()) {
         // Skip anonymous defs.
         if (jt.second->isAnonymous())
           continue;
 
         if (findUse(*jt.second, it.second->getDefInit(), hasUse)) {
           PrintWarning(jt.second->getLoc(),

mlir/test/mlir-tblgen/deprecation-transitive.td

+// RUN: mlir-tblgen -I %S/../../include %s 2>&1 | FileCheck %s --implicit-check-not warning:
+
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect : Dialect {
+  let name = "test_dialect";
+  let useFoldAPI = kEmitFoldAdaptorFolder;
+}
+
+def OpTraitA : NativeOpTrait<"OpTraitA">, Deprecated<"use `bar` instead">;
+
+// CHECK: :[[# @LINE + 1]]:{{[0-9]+}}: warning: Using deprecated def `OpTraitA`
+def A_AsField {
+  NativeOpTrait value = OpTraitA;
+}
+
+// CHECK: :[[# @LINE + 1]]:{{[0-9]+}}: warning: Using deprecated def `OpTraitA`
+def B_InList {
+  list<NativeOpTrait> value = [OpTraitA];
+}
+
+// CHECK: :[[# @LINE + 1]]:{{[0-9]+}}: warning: Using deprecated def `OpTraitA`
+def C_InListList {
+    list<list<NativeOpTrait>> value = [[OpTraitA]];
+}
+
+class Base;
+
+// CHECK: :[[# @LINE + 1]]:{{[0-9]+}}: warning: Using deprecated def `OpTraitA`
+def D_InDagAsOperator {
+  dag value = (OpTraitA $test);
+}
+
+// CHECK: :[[# @LINE + 1]]:{{[0-9]+}}: warning: Using deprecated def `OpTraitA`
+def E_InDagAsArg : Base {
+  dag value = (ins OpTraitA:$test);
+}
+
+class ThingTakingList<list<NativeOpTrait> l> {
+  list<NativeOpTrait> i = l; // avoid unused variable warning.
+}
+
+// CHECK: :[[# @LINE + 1]]:{{[0-9]+}}: warning: Using deprecated def `OpTraitA`
+def F_AsFieldWithinAnonymousDef {
+  ThingTakingList value = ThingTakingList<[OpTraitA]>;
+}
+
+// CHECK: :[[# @LINE + 1]]:{{[0-9]+}}: warning: Using deprecated def `OpTraitA`
+def G_InDagAsAnonymousDefOperator {
+  dag value = (ThingTakingList<[OpTraitA]> $test);
+}
+
+def H_ButNotTransitivelyInNonAnonymousDef {
+  Base value = E_InDagAsArg;
+}