This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] NFC: Refactor fusion of LinalgOp with TensorReshapeOp by expansion.
ClosedPublic

Authored by mravishankar on Dec 22 2020, 11:45 PM.

Details

Summary

Change the implementation of LinalgOp with TensorReshapeOp by
expansion to be more modular and easier to follow.

Depends On D93724

Diff Detail

Event Timeline

mravishankar created this revision.Dec 22 2020, 11:45 PM
mravishankar requested review of this revision.Dec 22 2020, 11:45 PM
hanchung requested changes to this revision.Dec 23 2020, 6:12 AM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
466–504

Look into the implementation, I feel this is more like a constructer, but you want to check if it construct successfully or not. The function assumes some things, e.g., reassociation is empty. I think if the function is expected to call only once, at least we should write it down to the documentation, or add example usage.

I suggest to do it in this way:

  1. Add a constructor which takes LinalgOp and fusedTensorIndex.
  2. Add something like std::call_once (or a flag) to compute, which make sure it is only called once.
  3. Add a comment to state the expected usage is call compute at least once...
521–522

Can't this be for (int64_t shape : expandedShape.drop_front())?

635

s/Region/region

657

auto is enough

669

auto is enough

This revision now requires changes to proceed.Dec 23 2020, 6:12 AM
mravishankar marked 3 inline comments as done.Dec 26 2020, 1:06 PM
mravishankar added inline comments.
mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
466–504

It is a constructor, but I dont know if there is a way to return an error from a constructor. AFAIK, thats not possible without exceptions. So keeping the constructor simple, and adding a compute method. Ill add comments and checks to verify that it either is called once, or that it resets state if called multiple times.

521–522

Thanks!

mravishankar marked an inline comment as done.

Rebase

hanchung requested changes to this revision.Dec 28 2020, 11:14 AM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
466–504

Yes, I think we typically don't return an error or exception from a constructor, and we should avoid that. The suggestion actually keeps the constructor simple, it only takes two variables and store them. The compute function is also simple, only need an extra bool variable to make sure it's called once.

I think both are okay, this doesn't make things harder since it's only used in this file, and one case.

nit: how about rename it with init?

657

missed this one? I would prefer at least l.660 and l.672 being consistent, ie, both use unsigned or both use auto.

This revision now requires changes to proceed.Dec 28 2020, 11:14 AM
mravishankar added inline comments.Jan 2 2021, 11:02 AM
mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
466–504

If its fine by you, I would prefer to leave it as is for now. It is defined and used in the same file, and adapting it to your recommendation is a minor refactoring. We can revisit this later.
I prefer compute cause it makes it explicit that this is computing the information needed for expansion.

657

Will make this auto. Yes, I missed this one. Thanks

Minor comment addressed

Addressed remaining minor comments

hanchung accepted this revision.Jan 8 2021, 11:36 AM
This revision is now accepted and ready to land.Jan 8 2021, 11:36 AM