Page MenuHomePhabricator

[mlir] Support inlining into affine operations
ClosedPublic

Authored by ftynse on Dec 7 2020, 9:07 AM.

Details

Summary

Introduce support for inlining into affine operations. This uses the generic
inline infrastructure and boils down to checking that, if applied, the inlining
doesn't violate the affine dimension/symbol value categorization. Given valid
IR, only the values that are valid dimensions/symbols thanks to being top-level
in their affine scope need special handling.

Diff Detail

Event Timeline

ftynse created this revision.Dec 7 2020, 9:07 AM
ftynse requested review of this revision.Dec 7 2020, 9:07 AM
rriddle accepted this revision.Dec 8 2020, 2:45 PM

Looks mostly fine to me, but will defer approval to those more closely following the affine dialect. (In case I forgot about some specific properties of the affine operations)

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
65

Is it still requiring ConstantOp directly and not using the more general m_Constant?

193

nit: You can merge all of these isa<> together: isa<AffineForOp, AffineParallelOp, ...>(parentOp)

This revision is now accepted and ready to land.Dec 8 2020, 2:45 PM
ftynse updated this revision to Diff 310446.Dec 9 2020, 12:51 AM
ftynse marked an inline comment as done.

Address review

Thanks for working on this, Alex! LGTM! Just some minor comments.

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
40

remmains -> remains

58–59

Maybe we could do:

if (value.isa<BlockArgument>())
  return legalityCheck

so that a legal block argument doesn't continue? The condition below will always be false for a BlockArgument. We could even move value.isa<OpResult>() to an assert.

mlir/test/Dialect/Affine/inlining.mlir
75

could you also please add a test with an affine.vector_load or affine.vector_store?

84

should we capture %arg0 and match it here to make sure the output is correct (similar to IV)?

Looks great!

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
140

The doc comment is missing documentation of wouldBeClone.

bondhugula added inline comments.Dec 10 2020, 12:11 AM
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
144–146

I might have gotten it wrong, but shouldn't be a check for the op having an AffineScope trait here?

144–146
  • shouldn't be -> shouldn't there be
ftynse updated this revision to Diff 310813.Dec 10 2020, 2:45 AM
ftynse marked 4 inline comments as done.

Address review

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
144–146

No, this is checking if we can inline into an affine control flow construct (for, if). These constructs are not affine scopes, but they are presumably in some scope already.

This revision was landed with ongoing or failed builds.Dec 11 2020, 7:24 AM
This revision was automatically updated to reflect the committed changes.