This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rriddle on Apr 21 2020, 3:21 PM.

Details

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

Diff Detail

Event Timeline

rriddle created this revision.Apr 21 2020, 3:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 3:21 PM

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
445

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

rriddle marked an inline comment as done.Apr 22 2020, 1:38 AM

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 updated this revision to Diff 259205.Apr 22 2020, 1:41 AM

Resolve comments

rriddle edited the summary of this revision. (Show Details)Apr 22 2020, 1:48 AM
bondhugula added inline comments.Apr 22 2020, 9:55 AM
mlir/lib/IR/SymbolTable.cpp
230

Nit: nested -> nestedOp

233

was a symbol table -> had a symbol table trait

mlir/lib/Transforms/SCCP.cpp
770

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 added inline comments.Apr 22 2020, 10:07 AM
mlir/lib/Transforms/SCCP.cpp
599

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

772

region-like -> return-like

773

return values -> call site results ?

793

Likewise.

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 updated this revision to Diff 259331.Apr 22 2020, 10:27 AM
rriddle marked 7 inline comments as done.

Resolve comments

rriddle edited the summary of this revision. (Show Details)Apr 22 2020, 10:28 AM
rriddle added inline comments.
mlir/lib/Transforms/SCCP.cpp
770

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.

Anything else here Uday?

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.

Anything else here Uday?

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

bondhugula requested changes to this revision.Apr 25 2020, 11:44 PM
bondhugula marked an inline comment as done.

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
91

Rephrase to clarify which uses are being referred to.

92

"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

385

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

394

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

410

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

411

which functions? You mean 'callables'?

412

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

564

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

573

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

579

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

773

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.

This revision now requires changes to proceed.Apr 25 2020, 11:44 PM
bondhugula marked an inline comment as done.Apr 26 2020, 1:29 AM
bondhugula added inline comments.
mlir/lib/Transforms/SCCP.cpp
410

I found this one among the test cases.

rriddle updated this revision to Diff 260251.Apr 27 2020, 2:32 AM
rriddle marked 24 inline comments as done.

Resolve comments

mlir/lib/Transforms/SCCP.cpp
579

Yes.

773

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 accepted this revision.Apr 27 2020, 2:47 AM
bondhugula added inline comments.
mlir/lib/Transforms/SCCP.cpp
773

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.

This revision is now accepted and ready to land.Apr 27 2020, 2:47 AM
rriddle updated this revision to Diff 260427.Apr 27 2020, 12:58 PM

Resolve comments

This revision was automatically updated to reflect the committed changes.