This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add maximum SSA form transformation pass
Needs ReviewPublic

Authored by mortbopet on Dec 2 2021, 4:51 AM.

Details

Summary

This pass converts a program into maximal SSA form. In this form, any value referenced within a block is also defined within the block. This is done by adding block arguments to all basic block dominance chains which may lead to an operation that relied on referencing a Value based on basic block dominance.

This pass is useful in dataflow-style programming models since it renders all data flow within the program explicit (through block arguments) instead of implicit (through block dominance).

This pass only works on Standard-level IR, in that it expects all operations (and blocks) within a FuncOp to be within the same region. Furthermore, it is assumed that any value referenced by any operation is eligible to be passed around as a block argument.

Diff Detail

Event Timeline

mortbopet created this revision.Dec 2 2021, 4:51 AM
mortbopet requested review of this revision.Dec 2 2021, 4:51 AM
mortbopet updated this revision to Diff 391279.Dec 2 2021, 4:53 AM

incorrect namespace

mortbopet updated this revision to Diff 391287.Dec 2 2021, 5:33 AM

missing }

mortbopet updated this revision to Diff 391382.Dec 2 2021, 10:28 AM

argument and control flow mappings should be stored for any value

rriddle requested changes to this revision.Dec 2 2021, 11:01 AM
rriddle added inline comments.
mlir/include/mlir/Transforms/Passes.td
656

This option is giving me a bad code smell. Why is memref special cased here?

rriddle added inline comments.Dec 2 2021, 11:01 AM
mlir/lib/Transforms/MaxSSA.cpp
15

Do you actually reference anything from here?

16

What is being used from here? Realistically, passes in Transforms/ shouldn't depend on any dialects.

This revision now requires changes to proceed.Dec 2 2021, 11:01 AM
mortbopet added inline comments.Dec 2 2021, 11:13 AM
mlir/include/mlir/Transforms/Passes.td
656

Fully agree. I am a bit unsure on how to go about creating an option for steering the exclusion of user-requested types. I guess an alternative could be an optional list of strings, wherein these strings are string-compared to known types in the pass, and base exclusion on that. But that would still require handling for every possible type that could be requested.

ListOption<"ignoredTypes", "ignore-types", "std::string",...>
...
bool isIgnored(Value v) {
  return llvm::TypeSwitch<Type, bool>(v.getType)
    .Case<MemRefType>([&](auto){
      return llvm::find(ignoredTypes, "memref") != ignoredTypes.end()
    })
    .Default([&](auto){return false;});
}

Do you know a better way of going about this?

mlir/lib/Transforms/MaxSSA.cpp
15

Can be removed.

16

BranchOpInterface; will convert to #include "mlir/Interfaces/ControlFlowInterfaces.h"

mortbopet updated this revision to Diff 391405.Dec 2 2021, 11:14 AM

update include

mehdi_amini added inline comments.Dec 2 2021, 4:20 PM
mlir/include/mlir/Transforms/Passes.td
656

What is the reason to not perform the transformations on memref values? Is this about the fact that it has pointer semantics? Would you want the same thing for the LLVM pointer type?

If so this is the kind of thing you can address with op-interfaces / type-interfaces.

mortbopet added inline comments.Dec 2 2021, 11:40 PM
mlir/include/mlir/Transforms/Passes.td
656

The specific use-case that i'm looking at is over in CIRCT where a pass is constrained to all memref's being statically determinable by either being function arguments or memref.alloc'ed in the entry block. This provides the motivation for somehow adding a hook into this pass to inform about certain types which should not be considered in SSA maximization - i think it would be best if this was done here, as to the alternative of maximizing all SSA values and then letting the user have to re-minimize things afterwards (which might be hard).

How do you see op/type interfaces being applicable to this problem, without unnecessarily polluting types with information specific to this single transformation?

mehdi_amini added inline comments.Dec 3 2021, 10:38 PM
mlir/include/mlir/Transforms/Passes.td
656

Interfaces (Op, Type, Attribute) are used to decouple the transformations from the entities (here a particular type).
Now it works only if there is an intrinsic property of the type that you could always use as discriminator to decide on whether to consider the type or not. But unfortunately it seems that your criteria is very much use-case specific, so interfaces won't be the right solution.

We're back to options: we'd need a serializable description of the configuration of the pass for that, a list of string could work even though it'll be limited in what it can describe.

bondhugula added inline comments.
mlir/test/Transforms/max_ssa.mlir
1 ↗(On Diff #391405)

Please use hyphens instead of underscores for the test cases.

This would be a really useful utility/pass to have! It would be good to (now or later) factor the core utility into MLIRTransformsUtils and call this from the pass. Some initial mostly minor comments.

mlir/include/mlir/Transforms/Passes.h
133

-> ... is either defined with the same block or is one of its block arguments?

mlir/include/mlir/Transforms/Passes.td
632

Nit: convert-to-max-ssa or maximize-ssa to make it a verb?

645

-> ... standard control flow ...

646

to be within -> to be nested immediately within the same region?

bondhugula added inline comments.
mlir/include/mlir/Transforms/Passes.td
656

You can have the pass creation method take a lambda filterTypes -> bool to filter out certain types or have a string list of fully-specified type names. In your case, builtin.memref would be passed. I think it's a useful option in general but with specialized use cases.

bondhugula added inline comments.Dec 4 2021, 4:16 AM
mlir/lib/Transforms/MaxSSA.cpp
60

operand

144

Please document the return status and when/why it might fail at a high level.

153

Why do you need an std::set instead of a DenseSet here?

195

Use /*..=*/ form for nullptr argument.

mehdi_amini added inline comments.Dec 4 2021, 11:07 AM
mlir/include/mlir/Transforms/Passes.td
656

A lambda breaks reproducibility, so I'd really like to avoid this.

If you need to inject C++ code, then make it a utility instead of a pass, and then it's up to the user to wrap it in a pass that injects the lambda to the utility, but at least that keeps the pass mechanism as a "standalone" thing.

bondhugula added inline comments.Dec 5 2021, 8:15 PM
mlir/include/mlir/Transforms/Passes.td
656

Right - the lambda could be on the utility. Something like this will invariably be factored into a utility + "a pass calling that utility" like a lot of the other transforms infrastructure. Then, the pass will have to take a list of string type names?

mortbopet updated this revision to Diff 391977.Dec 5 2021, 11:57 PM
mortbopet marked 3 inline comments as done.

address review comments

mortbopet updated this revision to Diff 391988.EditedDec 6 2021, 12:53 AM
mortbopet marked 2 inline comments as done.

Rework the conversion pass into a transform utility. In doing so, also define a test pass for the filtered case. The transformation utility now takes a lambda to specify a value filter. The user-exposed pass convert-to-max-ssa no longer takes any arguments. For my own use-case, this is sufficient, since i'll be using the SSA maximization utility with a filtered type as a precondition for another conversion pass.

rriddle requested changes to this revision.Dec 6 2021, 1:01 AM

Thanks, this works much better as a reusable utility.

mlir/include/mlir/Transforms/MaxSSAUtils.h
28

Does this need to be std::function? llvm::function_ref is generally a better choice if you only need a reference to a function (e.g. it doesn't allocate memory).

30–34

What aspects of this require being nested under FuncOp? This seems like a utility that could be written fairly generically over any root operation.

mlir/lib/Transforms/MaxSSA.cpp
29–30
36–40
mlir/lib/Transforms/Utils/MaxSSAUtils.cpp
118

Please use the global LogicalResult functions.

124

Same here and throughout.

188

Leftover debugging?

200
219–229
This revision now requires changes to proceed.Dec 6 2021, 1:01 AM
bondhugula added inline comments.Dec 6 2021, 2:02 AM
mlir/include/mlir/Transforms/MaxSSAUtils.h
30

Nit: Start with Converts a ....

30–34

Many region holding operations don't allow block arguments to be arbitrarily added to them -- some have very specific meanings for those (scf.for, affine.for, gpu.launch) and some may not even allow block arguments (scf.execute_region). This can perhaps be generalized to FunctionLike ops.

bondhugula added inline comments.Dec 6 2021, 2:05 AM
mlir/lib/Transforms/Utils/MaxSSAUtils.cpp
20

Unnecessary include here.

73–74

You aren't using this variable's value later (it's being rewritten). Please use llvm::is_contained(...).

94

auto alreadyinBlockValues = ...;

mortbopet marked 24 inline comments as done.Dec 6 2021, 9:56 AM
mortbopet added inline comments.
mlir/lib/Transforms/MaxSSA.cpp
153

Changed to DenseSet.

mlir/lib/Transforms/Utils/MaxSSAUtils.cpp
73–74

llvm::is_contained(...) doesn't seem to apply for llvm::DenseMap. Will use inBlockValues[v].count(use) == 0 instead.

mortbopet updated this revision to Diff 392116.Dec 6 2021, 9:57 AM
mortbopet marked 2 inline comments as done.

address review comments.

rriddle added inline comments.Dec 6 2021, 11:19 AM
mlir/include/mlir/Transforms/MaxSSAUtils.h
30–34

Right, the constraints here should likely be captured in a comment. In the future this would be better driven through interfaces/traits, rather than limiting to FunctionLike/FuncOp/etc.

rriddle added inline comments.Dec 6 2021, 1:31 PM
mlir/include/mlir/Transforms/MaxSSAUtils.h
63–78

This is missing the fact that the value is required to be defined by an operation within a FuncOp.

mlir/lib/Transforms/Utils/MaxSSAUtils.cpp
141–148

This seems more appropriate as an assert than an error.

mlir/test/Transforms/max-ssa-ignore.mlir
6–62

Can you trim this down to the smallest test you can create? This seems quite large for a focused test.

mlir/test/Transforms/max-ssa.mlir
113–196

What is this testing? It isn't clear that this is providing a focused test, and seemingly includes a lot of unnecessary operations.

mortbopet updated this revision to Diff 392387.Dec 7 2021, 7:08 AM

add missing early_inc_range and simplify tests

mortbopet updated this revision to Diff 392392.Dec 7 2021, 7:13 AM
mortbopet marked 2 inline comments as done.

address review comments

bondhugula added inline comments.Dec 7 2021, 7:59 AM
mlir/lib/Transforms/Utils/MaxSSAUtils.cpp
45

Please name this replaceAllUsesInBlockWith. Consider aligning the signature of this with mlir::replaceAllUsesInRegionWith. This can even be moved to RegionUtils.cpp if desired.

142

You can't use the result of getDefiningOp unchecked. You are missing an assert or this is a bug.

149

For private methods, I think it's a convention to have the doc comment on the function definition.

173

We need a comment here or below.

mortbopet updated this revision to Diff 392411.Dec 7 2021, 8:09 AM

reduce number of times each block is visited

rriddle added inline comments.Dec 7 2021, 10:15 AM
mlir/lib/Transforms/MaxSSA.cpp
37–41
mlir/lib/Transforms/Utils/MaxSSAUtils.cpp
141
mlir/test/lib/Transforms/TestMaxSSAFiltered.cpp
2

nit: Please name this the same more generically as the pass itself, just in case we add more test passes related to the pass here.

mortbopet updated this revision to Diff 393811.Dec 13 2021, 1:03 AM
mortbopet marked 7 inline comments as done.

Address review comments.

I'm deferring to River here given the more detailed review he has done. Just making sure this isn't waiting for me.

@rriddle do you have anything else you'd like too see done for the PR?

mlir/lib/Transforms/Utils/MaxSSAUtils.cpp
142

Thank you for catching that; rewriting it so it can be used with block arguments as well.

mlir/test/Transforms/max-ssa-ignore.mlir
6–62

Updated test cases (in max-ssa.mlir) to mostly contain control flow operators, and pass around values defined in the entry block.

rriddle added inline comments.Jan 9 2022, 10:57 PM
mlir/include/mlir/Transforms/MaxSSAUtils.h
29

nit: You can drop the llvm:: here.

65
mlir/lib/Transforms/Utils/MaxSSAUtils.cpp
35–38

Is this assuming that a branch only has unique successors? That isn't the case in MLIR, e.g. a conditional branch can go to the same block in both the true and false case.

91

Does this need to be inserted after? or could you fuse this into the if above? (i.e. call insert and check the return)

206

Only the class should be in the anonymous namespace, the functions should be at top-scope.

mlir/lib/Transforms/Utils/RegionUtils.cpp
34–36 ↗(On Diff #393811)

This could be implemented using Value::replaceUsesWithIf.