[PATCH] D125212: [GlobalISel] Implement the Has{No}Use builtin predicates

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/test/TableGen/GlobalISelEmitter-HasUse.td:45
+
+let HasUse = true in
+defm atomic_load_add_ret : binary_atomic_op<atomic_load_add>;
----------------
foad wrote:
> A philosophical question: do we really need HasUse? After all the atomic_load_add_ret pattern will still work in both cases (use & nouse), so shouldn't we use priorities instead to prefer the atomic_load_add_no_ret pattern when both are applicable?
Yes, having one predicate and priorities would be more consistent


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:4037-4042
+    // FIXME: This should be part of addBuiltinPredicates(). If we add this at
+    // the start of addBuiltinPredicates() without returning, then there might
+    // be cases where we hit the last return before which the
+    // HasAddedBuiltinMatcher will be set to false. The predicate could be
+    // missed if we add it in the middle or at the end due to return statements
+    // after the addPredicate<>() calls.
----------------
It's very not obvious but the code is structured in such a way that assumes each level of PatFrag you define only defines one predicate. We need verification (like the DAG path has) that this is the case


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:5271
+      if ((UsePred = dyn_cast<UsePredicateMatcher>(Pred.get()))) {
+        if (UsePred->hasUse()) {
+          return failedImport("Src pattern result has " +
----------------
The naming here is potentially confusing, since it looks like it's checking for a use of the predicate itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125212