Page MenuHomePhabricator

[MLIR] Vector store to load forwarding
ClosedPublic

Authored by anandkodnani on Wed, Jul 22, 3:09 AM.

Details

Summary
The MemRefDataFlow pass does store to load forwarding
only for affine store/loads. This patch updates the pass
to use affine read/write interface which enables vector
forwarding.

Diff Detail

Event Timeline

anandkodnani created this revision.Wed, Jul 22, 3:09 AM

just a few fly by comment on test style

mlir/test/Transforms/memref-vector-dataflow.mlir
3 ↗(On Diff #279810)

single white space line

19 ↗(On Diff #279810)

why are these comment here?

53 ↗(On Diff #279810)

if you want to match closing, then perhaps also add opening, and all closing patterns?

bondhugula requested changes to this revision.Wed, Jul 22, 11:28 AM

This looks good - thanks for contributing. A bunch of minor doc related comments.

mlir/include/mlir/Dialect/Affine/IR/AffineOps.td
728–729

Nit: stored value -> value to be stored

mlir/test/Transforms/memref-vector-dataflow.mlir
2 ↗(On Diff #279810)

Please add a line on what these test cases are testing.

32–35 ↗(On Diff #279810)

Please indent suitably.

50–51 ↗(On Diff #279810)

You may want to capture the store value and check if the add operand is that.

53 ↗(On Diff #279810)

CHECK-NEXT here?

This revision now requires changes to proceed.Wed, Jul 22, 11:28 AM

The second line in the commit summary is broken. Please rephrase. Also,

only for scalars -> only for affine store/loads

bondhugula retitled this revision from Vector store to load forwarding to [MLIR] Vector store to load forwarding.Wed, Jul 22, 11:30 AM

Sorry, I've taken the liberty to add the MLIR tag to the commit title - to avoid any ambiguity w.r.t LLVM proper.

anandkodnani edited the summary of this revision. (Show Details)

Addressed comments on test and commit message.

anandkodnani marked 9 inline comments as done.Thu, Jul 23, 4:14 AM
anandkodnani added inline comments.
mlir/test/Transforms/memref-vector-dataflow.mlir
19 ↗(On Diff #279810)

Updated the test case.

anandkodnani marked an inline comment as done.Thu, Jul 23, 4:15 AM
ftynse accepted this revision.Thu, Jul 23, 4:26 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
85

I don't understand what "underlying value" means in this context. Do you mean the value read by this operation?

91

Nit: return cast<ConcreteOp>(this->getOperaiton()); looks more concise. return this->getOperation()->getResult(0); looks equally concise and arguably clearer about the intent as opposed to an explicit cast to ConcreteOp followed by an implicit cast to Value.

mlir/test/Transforms/memref-vector-dataflow.mlir
1 ↗(On Diff #280071)

Why can't this live in mlir/test/Transforms/memref-dataflow-opt.mlir?

  • Updated getValue method in interface.
  • Moved the test to memref-dataflow-opt.mlir
anandkodnani marked 3 inline comments as done.Thu, Jul 23, 5:27 AM
anandkodnani added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
85

I meant the value represented by the interface.

91

Using return cast<ConcreteOp>(this->getOperaiton()); just to keep consistent style with other methods in the interface.

anandkodnani marked an inline comment as done.Thu, Jul 23, 5:28 AM
ftynse added inline comments.Thu, Jul 23, 5:35 AM
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
85

These are _operation_ interfaces, they don't represent values. The fact that single-result operations are implicitly convertible to values should not affect this. So "the value read by this operation" sounds like the right formulation here.

anandkodnani marked an inline comment as done.
dcaballe added inline comments.Thu, Jul 23, 11:08 AM
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
87

getValue is something we could get from the OneResult trait. However, I was playing with this, adding OneResult trait to this interfaces and I couldn't make it work. @ftynse or anybody else, do you know if this is possible?

bondhugula added inline comments.Sat, Jul 25, 11:33 AM
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
87

The OneResult should automatically be added by the generator. You could verify this. Doing .getResult() should already give you the loaded value, right? Why do we need this?

bondhugula accepted this revision.Sat, Jul 25, 11:36 AM
bondhugula added inline comments.
mlir/test/Transforms/memref-dataflow-opt.mlir
284

Nit: ... value forwarding from vector stores to vector loads.

This revision is now accepted and ready to land.Sat, Jul 25, 11:36 AM
anandkodnani marked an inline comment as done.
ftynse added inline comments.Mon, Jul 27, 12:57 AM
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
87

@ftynse or anybody else, do you know if this is possible?

I don't think there is an easy way. Some time ago, I had a prototype that inherited the interface trait class from another trait class (for FunctionLike), but the code to support that was quite convoluted. That being said, having cast<ConcreteOp>(this->getOperation().getResult() should dispatch properly, without having to know if ConcreteOp has the trait or if it got the method from elsewhere. If you want just a check, something like static_assert(std::is_base_of<InterfaceName::Trait, ConcreteOp>::value, "") should do the trick.

dcaballe accepted this revision.Mon, Jul 27, 10:32 PM
dcaballe added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td
87

The OneResult should automatically be added by the generator. You could verify this. Doing .getResult() should already give you the loaded value, right? Why do we need this?

The OneResult trait is automatically added to the concrete ops but not to the interfaces. My hope was that we could add OneResult to the interface and then get rid of the getValue() method altogether and replace it with .getResult() instead. However, that doesn't seem to be possible right now so... fair enough! Thanks for the info!

This revision was automatically updated to reflect the committed changes.