This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add a ControlFlowSink pass.
ClosedPublic

Authored by Mogball on Dec 3 2021, 5:24 PM.

Details

Summary

Control-Flow Sink moves operations whose only uses are in conditionally-executed regions into those regions so that paths in which their results are not needed do not perform unnecessary computation.

Depends on D115087

Diff Detail

Event Timeline

Mogball created this revision.Dec 3 2021, 5:24 PM
Mogball requested review of this revision.Dec 3 2021, 5:24 PM
mehdi_amini added inline comments.Dec 4 2021, 4:13 PM
mlir/include/mlir/Transforms/Passes.td
315

Isn't this the opposite of LICM?

Mogball added inline comments.Dec 6 2021, 8:43 AM
mlir/include/mlir/Transforms/Passes.td
315

Basically, yeah.

jpienaar added inline comments.Dec 6 2021, 8:51 AM
mlir/include/mlir/Transforms/Passes.td
315

At least wrt loops (this is peer of https://llvm.org/docs/Passes.html#sink-code-sinking)

Mogball updated this revision to Diff 392108.Dec 6 2021, 9:41 AM

Fixing the build and updating the pass description.

Mogball updated this revision to Diff 392109.Dec 6 2021, 9:42 AM

Corrected the diff.

mehdi_amini requested changes to this revision.Dec 6 2021, 1:45 PM

I'm not sure about the interest of such a pass, without some profitability aspects.

I guess this could be a test pass, where the core of the transformation is exposed as a utility. The utility could have some filtering / callback to support the decision process and users could then implement the pass for their own needs.

This revision now requires changes to proceed.Dec 6 2021, 1:45 PM

I'm not sure about the interest of such a pass, without some profitability aspects.

I guess this could be a test pass, where the core of the transformation is exposed as a utility. The utility could have some filtering / callback to support the decision process and users could then implement the pass for their own needs.

I'm not against this approach, as I have a specific use-case in mind (TF) that will probably require the API to be modified, but I have to yet decide what exactly the callbacks will be. However, the pass as-is should be a uniform gain, because it only considers non-memory-effecting ops.

This revision now requires review to proceed.Dec 6 2021, 2:16 PM

I guess the "single conditionally-executed region" may be enough of a criteria here to be conservative and get started. I suspect we'll still need to evolve this to be more configurable, so I'd rather have the split between the utility and the pass up-front (even if the utility doesn't expose options immediately)

Mogball updated this revision to Diff 392217.Dec 6 2021, 4:10 PM

Simplify the implementation.

rriddle requested changes to this revision.Dec 7 2021, 10:35 AM
rriddle added inline comments.
mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
91–92

Is a magic number necessary? Why not use Optional to represent unknown? Seems safer and less prone to error.

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
154

Can we not use something like $_op here?

156

nit: Spell out auto here.

rriddle added inline comments.Dec 7 2021, 10:35 AM
mlir/lib/Transforms/ControlFlowSink.cpp
39–42

Can you separate the pass and utilty? similarly to how it's done in https://reviews.llvm.org/D114945

45–47

What does the failure represent here? Also, why are you stipulating only r-reference. This is a local utility class, you don't need to add that.

108–112

Seems like this is essentially just checking if all users are nested within region? Seems like that could be checked without the need of DominanceInfo.

118
119
123–124
141

If you want a stack, prefer using SmallVector instead.

142–144
148

You can merge the depth first comment into the comment before the loop.

154

Why the FailureOr? This always succeeds.

156–157
159

This should already be guaranteed by the infra, this situation can't happen in valid IR.

189

Do you need a LogicalResult return? This always succeeds.

This revision now requires changes to proceed.Dec 7 2021, 10:35 AM
Mogball marked 2 inline comments as done.Dec 7 2021, 11:20 AM
Mogball added inline comments.
mlir/lib/Transforms/ControlFlowSink.cpp
45–47

Adding && made more sense when I had data structures stored in the struct (which I've since removed by simplifying the logic) that made the utility struct "single-use". I can drop it now.

108–112

What happens if the user is in a block unreachable from the entry block? Then moving the op to the entry block would result in invalid IR.

141

I used a deque after seeing this

154

I thought maybe I'd add LogicalResult in case the pass might fail in the future. But now that I think about it, I don't think it will. I'll drop them.

159

Ack

bondhugula requested changes to this revision.Dec 10 2021, 10:44 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/Transforms/ControlFlowSink.h
12 ↗(On Diff #392217)

It is out of line to have a method with this name exposed on mlir:: -- the name isn't descriptive enough for this namespace as well even if this needs to be exposed.

mlir/lib/Transforms/ControlFlowSink.cpp
199–209

All code being called from here can just go into a utility in lib/Transforms/Utils/Utils.cpp?

LogicalResult mlir::controlFlowSink(RegionBranchOpInterface branchOp, DominanceInfo &domInfo) {
   ...
}

You can return the number of things that have been sunk if desired instead of LogicalResult. I'm also not sure that a failure of this utility should be the cause for a pass failure. Is the IR going to be invalid? If so, sinkRegions and/or sinkConditionalCode should be documenting that which they don't appear to.

Mogball marked 12 inline comments as done.Jan 13 2022, 8:13 PM
Mogball updated this revision to Diff 400405.Jan 16 2022, 2:23 PM

Review comments and separating utility from pass

I still see several comments that haven't been marked "Done".

mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
88

"defined" or "known"?

106

A comment here that a None is returned for an unknown upper bound?

mlir/include/mlir/Transforms/ControlFlowSink.h
12 ↗(On Diff #392217)

@Mogball Did you missing updating the revision?

Mogball marked 9 inline comments as done.Jan 18 2022, 3:08 PM

thanks!

mlir/include/mlir/Transforms/ControlFlowSink.h
12 ↗(On Diff #392217)

Ah yeah I did.

Mogball updated this revision to Diff 401008.Jan 18 2022, 3:08 PM
Mogball marked an inline comment as done.

Update the diff

bondhugula added inline comments.Jan 18 2022, 5:04 PM
mlir/lib/Transforms/ControlFlowSink.cpp
16

This file is missing its primary include of Passes.h. In fact, mlir/Transforms/PassDetail.h is layered completely wrong -- it shouldn't be including Passes.h.

mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
110–120

Why do you need a deque here? You are only pushing and popping at the end.

bondhugula added inline comments.Jan 18 2022, 5:08 PM
mlir/lib/Transforms/ControlFlowSink.cpp
16

Looks like mlir::FusionMode is needed for PassDetail.h and Passes.h was included there. That can be cleaned up as part of a separate refactoring.

mehdi_amini added inline comments.Jan 18 2022, 6:37 PM
mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
110–120

This is referring to a micro-benchmark posted on the mailing list in 2012, do you have more recent data?

Right now when I run the benchmark on my Linux box I get:

Stack:
accum = 636
time = 4.486544 seconds


Deque:
accum = 636
time = 5.598040 seconds


Deque iter insert:
accum = 636
time = 10.433442 seconds


Vector:
accum = 636
time = 2.632639 seconds


List:
accum = 636
time = 31.212231 seconds

(clang++ -O3)

At the time they reported:

List: 92.473490 seconds
Stack: 15.575143 seconds
Deque: 9.129154 seconds
Deque iter insert: 7.384741 seconds
Vector: 13.387473 seconds
rriddle added inline comments.Jan 18 2022, 10:19 PM
mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
151

Might be good to add a helper static InvocationBounds InvocationBounds::getUnknown() method that makes it easier to do this.

mlir/include/mlir/Transforms/Utils.h
21

Can you forward declare the classes you need instead of adding an include?

mlir/lib/Transforms/ControlFlowSink.cpp
60

Is the mlir:: here necessary?

mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
124–128
mlir/test/Transforms/control-flow-sink.mlir
7

Can you use proper file check variables for all of these SSA captures? Ideally the tests should only be testing the minimum necessary.

https://mlir.llvm.org/getting_started/TestingGuide/

(Same for the other tests)

Mogball added inline comments.Jan 18 2022, 11:11 PM
mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
110–120

I do not. It may be worth changing the one in CSE to a vector too then!

Mogball updated this revision to Diff 401483.Jan 19 2022, 7:38 PM
Mogball marked 8 inline comments as done.

review comments

Mogball updated this revision to Diff 401484.EditedJan 19 2022, 7:48 PM

actually update the diff

jpienaar added inline comments.Jan 19 2022, 8:03 PM
mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
141

Does this need an update?

mlir/include/mlir/Transforms/Passes.td
319

similar but opposite? :-)

mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
94

The mix of Operation& and Operation* I find distracting, I'd just use Operation* throughout as that's the convention elsewhere

148

The name of the pass is more general than what it does, while the comments all refer to this being simple. Is the intention to generalize the pass later (and so the name) and these are just documenting the current state? Or is the intention to have a simple sinking pass and there be others?

150

Could we use llvm::zip?

Mogball updated this revision to Diff 401516.Jan 19 2022, 10:58 PM
Mogball marked 5 inline comments as done.

review comments

Mogball added inline comments.Jan 20 2022, 9:55 AM
mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
148

It's meant to be generalized later such that the default cost model is the "simple" case (i.e. no duplicating ops). I'll make that clear in the documentation.

rriddle accepted this revision.Jan 20 2022, 9:37 PM

Did a scan and looks good from my point of view as something that we can land and iterate on.

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
151

Can we use it here ;)

bondhugula accepted this revision.Jan 22 2022, 8:38 PM

LGTM.

Nit: "work queue" -> "work list" at many places.

mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
44

Why is this returning an rvalue ref for a POD? Haven't seen this pattern in the tree.

This revision is now accepted and ready to land.Jan 22 2022, 8:38 PM
jpienaar accepted this revision.Jan 24 2022, 7:29 AM

Nice, thanks for refactoring!

mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
87

is always known and at least 0 ?

Or is this trying to say it may be unknown but in that case 0 could be returned so there'll always be a lower bound even if not exact? (I think the latter)

mlir/include/mlir/Transforms/Passes.td
313

I'd split this into two and remove simple here now. First say what the pass is doing and then say it's restrictions (which currently is a very conservative cost model)

mlir/lib/Transforms/ControlFlowSink.cpp
26

Some of these now feel like properties of the helper methods rather than this pass.

mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
10

utilities

97

And this is reason that the dominance isn't updated? But the dominance information cannot be marked as retained as this is only true locally (e.g., within cf op) [just checking my understanding what preserved means here]

110–120

Create a GitHub tracking issue for it :-)

141

(void) to make it clear whether matches or not doesn't matter here

142

Nit: new line above (i was reading this as one clump and wondering why bounds were being called inside the loop :-))

mlir/test/Transforms/control-flow-sink.mlir
2

Is split-input-file adding anything here? We mostly only use it for error case checking, it returns a less useful file location upon failure.

This revision was landed with ongoing or failed builds.Jan 24 2022, 3:08 PM
This revision was automatically updated to reflect the committed changes.

Ah wait I totally forgot about the other comments -- guess I'm sending an NFC .-.

Mogball marked 11 inline comments as done.Jan 24 2022, 3:30 PM
Mogball added inline comments.
mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
87

The lower bound is known to be at least zero (because you can't negatively execute a region...)

mlir/lib/Transforms/Utils/ControlFlowSinkUtils.cpp
97

Just locally in the subgraph. I'll update the comment.