[PATCH] D78592: [mlir][SCCP] Add support for propagating across symbol based calls

Sat Apr 25 23:57:08 PDT 2020

bondhugula requested changes to this revision.
bondhugula marked an inline comment as done.
bondhugula added a comment.
This revision now requires changes to proceed.

Looks great overall. A bunch of mostly minor comments on tweaking doc/comments. Feel free to ignore the minor ones if not necessary. Many of them are also due to the order in which I looked at the changes and "local" reading.

================
Comment at: mlir/include/mlir/IR/SymbolTable.h:91
+  /// and a boolean signifying if the symbols within that symbol table can be
+  /// treated as if all uses are visible. `allSymUsesVisible` identifies whether
+  /// all of the symbol uses of symbols within `op` are visible.
----------------
Rephrase to clarify which uses are being referred to. 

================
Comment at: mlir/include/mlir/IR/SymbolTable.h:92
+  /// treated as if all uses are visible. `allSymUsesVisible` identifies whether
+  /// all of the symbol uses of symbols within `op` are visible.
+  static void walkSymbolTables(Operation *op, bool allSymUsesVisible,
----------------
"visible" <- clarify as to visible to whom? (the walk?)

all uses -> all their uses?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:120
+/// This class contains various state used when computing the lattice of
+/// callable operation.
+class CallableLatticeState {
----------------
Nit: "a callable operation".

================
Comment at: mlir/lib/Transforms/SCCP.cpp:124
+  /// Build a lattice state with a given callable region, and a specified number
+  /// of results.
+  CallableLatticeState(Region *callableRegion, unsigned numResults)
----------------
of results. -> of results initialized to the default lattice value (Unknown). 

================
Comment at: mlir/lib/Transforms/SCCP.cpp:139
+
+  /// Add a call to this callable. This is only used if the callable defines a
+  /// symbol.
----------------
Are there examples of callables that don't define symbols in the codebase?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:143
+
+  /// Return the calls that are referenced by this callable. This is only used
+  /// if the callable defines a symbol.
----------------
calls referenced by callables? Did you mean "calls referencing this callable"? Also, shouldn't this just return empty if the callable doesn't define a symbol?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:151
+
+  /// The lattice state for each of the results of this region. The results of
+  /// the callable don't have proper SSA values, so we need to track them
----------------
Nit: results -> return values?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:152
+  /// The lattice state for each of the results of this region. The results of
+  /// the callable don't have proper SSA values, so we need to track them
+  /// separately.
----------------
don't have proper SSA values -> aren't SSA values? (since the parent op is declarative)?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:179
 private:
+  /// Initialize the set of symbol callables that can have their arguments and
+  /// results tracked. 'op' is the top-level operation that SCCP is operating
----------------
Nit: symbol callables -> symbol defining callables

================
Comment at: mlir/lib/Transforms/SCCP.cpp:385
+  // Initialize the set of symbol callables that can have their state tracked.
+  // This tracks which symbol callable operations we can propagate within.
+  auto walkFn = [&](Operation *symTable, bool allUsesVisible) {
----------------
Did you just mean "propagate within" or "propagate within and out of"?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:394
+        continue;
+      // We only care about symbol callables here.
+      auto symbol = dyn_cast<SymbolOpInterface>(callable.getOperation());
----------------
Nit: symbol callables -> symbol defining callables (I know that 'symbol' has now almost been established as an adjective for 'callable' in most of the code, but this may still be useful for new reviewers/readers.)

================
Comment at: mlir/lib/Transforms/SCCP.cpp:410
+    // After computing the valid callables, walk any symbol uses to check
+    // for non-call references. We won't be able to track the lattice state
+    // for arguments to these functions, as we can't guarantee that we can
----------------
Do you have a test case for non-call references?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:411
+    // for non-call references. We won't be able to track the lattice state
+    // for arguments to these functions, as we can't guarantee that we can
+    // see all of the calls.
----------------
which functions? You mean 'callables'?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:412
+    // for arguments to these functions, as we can't guarantee that we can
+    // see all of the calls.
+    Optional<SymbolTable::UseRange> uses =
----------------
Nit: all of the calls -> all of its calls

================
Comment at: mlir/lib/Transforms/SCCP.cpp:564
+  // TODO: Add support for non-symbol callables when necessary. This is simply
+  // detecting if there are non-call uses.
+  markAllOverdefined(op, op->getResults());
----------------
The second statement "This is simply detecting ..." is not really clear. What's the connection b/w non-symbol callables and non-call uses?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:573
+  Operation *callableOp = nullptr;
+  if (Value callableValue = op.getCallableForCallee().dyn_cast<Value>())
+    callableOp = callableValue.getDefiningOp();
----------------
I think the CallOpInterface / getCallableForCallee comment should have a line explaining why the callee (CallInterfaceCallable) can be an SSA value.

================
Comment at: mlir/lib/Transforms/SCCP.cpp:579
+  // The callable of this call can't be resolved, mark any results overdefined.
+  if (!callableOp)
+    return markAllOverdefined(op, callResults);
----------------
Will the default constructed value from 'lookup' above be nullptr?

================
Comment at: mlir/lib/Transforms/SCCP.cpp:774
+  // If this terminator is not "return-like", conservatively mark all of the
+  // call-site results as overdefined.
+  auto callableResultLattices = latticeIt->second.getResultLatticeValues();
----------------
But this terminator might not be the only terminator in the region of the callable op. Why mark call site results overdefined? You'd just propagate to successors the same way as for non-callable op region terminators that aren't return-like. ? You do have a test case ("complex_inner_if") where you have a cond_br in the callable. I was expecting that to not work as a result of this.

================
Comment at: mlir/test/Transforms/sccp-callgraph.mlir:68
+    // NESTED: %[[CALL:.*]]:2 = call @nested
+    // NESTEDNESTED: return %[[CST]], %[[CALL]]#1 : i32, i32
+
----------------
Prefix repetition. 

================
Comment at: mlir/test/Transforms/sccp-callgraph.mlir:106
+
+// CHECK-LABEL: func @non_call_users(
+func @non_call_users() -> (i32, i32) {
----------------
But this function a 'call user'. The non-call user is further below. 

================
Comment at: mlir/test/Transforms/sccp-callgraph.mlir:183
+
+func @complex_cond() -> i1
+
----------------
Nit: Move this below the next function - closer to its use. 

================
Comment at: mlir/test/Transforms/sccp-callgraph.mlir:232
+// CHECK-LABEL: func @complex_caller(
+func @complex_caller(%arg0 : i32) -> i32 {
+  // CHECK: %[[CST:.*]] = constant 1 : i32
----------------
This is a nice test case. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78592