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

[rriddle]

Summary

This revision adds support for propagating constants across symbol-based callgraph edges. It uses the existing Call/CallableOpInterfaces to detect the dataflow edges, and propagates constants through arguments and out of returns.

Depends On D78522

[bondhugula]

Looks great!

It assumes that the arguments of callable can be tracked so long as we know that all of the uses are calls.

When can the arguments not be tracked? And shouldn't the CallableOpInterface guarantee that?

mlir/lib/Transforms/SCCP.cpp

446

Nit: `/*...=*/!op->getBlock()

[rriddle]

In D78592#1996224, @bondhugula wrote:

Looks great!

It assumes that the arguments of callable can be tracked so long as we know that all of the uses are calls.

When can the arguments not be tracked? And shouldn't the CallableOpInterface guarantee that?

I thought I had a reason originally, but after thinking about it the constraints of symbol visibility already guarantee this today.

[rriddle]

Resolve comments

[bondhugula]

mlir/lib/IR/SymbolTable.cpp

230

Nit: nested -> nestedOp

233

was a symbol table -> had a symbol table trait

mlir/lib/Transforms/SCCP.cpp

774

Callable things can never have results, right? Can't you assert for the trait instead? Also, this should be moved to before the check right above.

[bondhugula]

mlir/lib/Transforms/SCCP.cpp

601

/*owner=*/ /*to=*/ /*from=*/

776

region-like -> return-like

777

return values -> call site results ?

797

Likewise.

[bondhugula]

adds some initial support for propagating constants across callgraph edges

Could you say a line or two on what 'initial' is or what other things you want to add in the near term?

[rriddle]

Resolve comments

[rriddle]

mlir/lib/Transforms/SCCP.cpp

774

non-symbol defining callables return the callable as a result value. This check is intended to intended to guard against the missing support for those callables.

[rriddle]

Anything else here Uday?

[bondhugula]

In D78592#1996277, @rriddle wrote:

In D78592#1996224, @bondhugula wrote:

Looks great!

It assumes that the arguments of callable can be tracked so long as we know that all of the uses are calls.

When can the arguments not be tracked? And shouldn't the CallableOpInterface guarantee that?

I thought I had a reason originally, but after thinking about it the constraints of symbol visibility already guarantee this today.

It'd be good to incorporate this understanding somewhere as a comment.

In D78592#2002333, @rriddle wrote:

Anything else here Uday?

Sorry about the delay - I should be able to get back on this by later today.

[bondhugula]

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.

mlir/include/mlir/IR/SymbolTable.h

92

Rephrase to clarify which uses are being referred to.

93

"visible" <- clarify as to visible to whom? (the walk?)

all uses -> all their uses?

mlir/lib/Transforms/SCCP.cpp

120

Nit: "a callable operation".

124

of results. -> of results initialized to the default lattice value (Unknown).

139

Are there examples of callables that don't define symbols in the codebase?

143

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?

151

Nit: results -> return values?

152

don't have proper SSA values -> aren't SSA values? (since the parent op is declarative)?

179

Nit: symbol callables -> symbol defining callables

386

Did you just mean "propagate within" or "propagate within and out of"?

395

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.)

411

Do you have a test case for non-call references?

412

which functions? You mean 'callables'?

413

Nit: all of the calls -> all of its calls

566

The second statement "This is simply detecting ..." is not really clear. What's the connection b/w non-symbol callables and non-call uses?

575

I think the CallOpInterface / getCallableForCallee comment should have a line explaining why the callee (CallInterfaceCallable) can be an SSA value.

581

Will the default constructed value from 'lookup' above be nullptr?

777

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.

mlir/test/Transforms/sccp-callgraph.mlir

69

Prefix repetition.

107

But this function a 'call user'. The non-call user is further below.

184

Nit: Move this below the next function - closer to its use.

233

This is a nice test case.

[bondhugula]

mlir/lib/Transforms/SCCP.cpp

411

I found this one among the test cases.

[rriddle]

Resolve comments

mlir/lib/Transforms/SCCP.cpp

581

Yes.

777

This branch is only taken when there are no CFG successors. This is the same behavior as the region case, i.e. if we see an "exiting" terminator that isn't return-like we assume all exit values are overdefined.

mlir/test/Transforms/sccp-callgraph.mlir

107

The function name is more of a test case name.

[bondhugula]

mlir/lib/Transforms/SCCP.cpp

777

I now see that far above at line 714 - the conditional over the call itself. Locally, this would be misleading including the method name visitCallableTerminatorOperation since these are for term ops that have 0 successors. Could you either update the doc comment or have a comment here to remind the reader? Adding ZeroSucc to the method name will be too long.

[rriddle]

Resolve comments