Page MenuHomePhabricator

[mlir] Change scf::LoopNest to store 'results'.
ClosedPublic

Authored by csigg on Oct 28 2022, 1:31 AM.

Details

Summary

This fixes the case where scf::LoopNest::loops is empty.

Change LoopVector and ValueVector to SmallVector.

Diff Detail

Event Timeline

csigg created this revision.Oct 28 2022, 1:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2022, 1:31 AM
csigg requested review of this revision.Oct 28 2022, 1:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2022, 1:31 AM
mlir/include/mlir/Dialect/SCF/IR/SCF.h
66–67

hmm .. who are the clients of this?

I am thinking that whoever is the client needs to either check !loops.empty or ValueRange != null, so whatever the case some check is needed at the client.
From an API perspective, it seems confusing to say that the you can take the result of an empty loop nest, I'd expect an assert(!loops.empty) here.

Is there another tradeoff I am not yet seeing that led you to this API change?

csigg added inline comments.Oct 28 2022, 2:15 AM
mlir/include/mlir/Dialect/SCF/IR/SCF.h
66–67

I expect that I can default-construct a LoopNest instance and call getResults() without crashing.

My particular use-case is that we call buildLoopNest() with empty lbs and empty iterArgs, which returns such a default-constructed LoopNest. One could argue that buildLoopNest().getResults() should return the ValueVector (which imho should be a SmallVector instead of a std::vector like everywhere else in MLIR) of the builder in that case, but that's an orthogonal issue.

csigg updated this revision to Diff 471498.Oct 28 2022, 5:03 AM

Add 'results' member to LoopNest.

csigg retitled this revision from [mlir] Fix empty scf::LoopNest. to [mlir] Change scf::LoopNest to store 'results'..Oct 28 2022, 5:04 AM
csigg edited the summary of this revision. (Show Details)
csigg added inline comments.
mlir/include/mlir/Dialect/SCF/IR/SCF.h
66–67

I changed this revision to include a 'results' vector in LoopNest to handle the empty lbs case properly.

csigg added a comment.Nov 4 2022, 2:57 AM

Nicolas, could you take another look please?

nicolasvasilache requested changes to this revision.Nov 10 2022, 6:47 AM

This is missing a test.

mlir/include/mlir/Dialect/SCF/IR/SCF.h
64–65

Thanks for cleaning this up.

66–67

I am sorry but I don't think that is a good change, you are now introducing opportunities to have a decoupling between loops and results that does not make sense to me..

mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
824

It is hard to see from this PR without tests whether this the one place where you see failures.

If so, why not just:

if (nest.loops.empty())
  return failure();
rewriter.replaceOp(op, nest.getResults());

?

The absence of a LoopNest is not the same as a loop nest without results ..

This revision now requires changes to proceed.Nov 10 2022, 6:47 AM
mlir/include/mlir/Dialect/SCF/IR/SCF.h
66–67
My particular use-case is that we call buildLoopNest() with empty lbs and empty iterArgs, which returns such a default-constructed LoopNest. One could argue that buildLoopNest().getResults() should return the ValueVector (which imho should be a SmallVector instead of a std::vector like everywhere else in MLIR) of the builder in that case, but that's an orthogonal issue.

I could see the APIs evolving to returning a FailureOr<LoopNest> and dropping the default constructor / enforcing non-empty loop nest at construction.

mlir/lib/Dialect/SCF/IR/SCF.cpp
526

Just make the API return FailureOr<LoopNest> / llvm::None ?

csigg added inline comments.Nov 10 2022, 6:59 AM
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
824

This is not where I saw the failure, this change is just necessary because I changed LoopNest's result from a member function to a member.

mlir/lib/Dialect/SCF/IR/SCF.cpp
526

That would call the bodyBuilder, create IR, and then return Failure? That sounds unintuitive.

mlir/lib/Dialect/SCF/IR/SCF.cpp
526

I wouldn't call it here but just return failure.
Maybe this corner case has been used as a "way to inline" the computation.

This code is quite old, seems like an opportunity to refactor to force users to call inline when appropriate and have the LoopNest building part either build a loop nest or fail as one would expect.

Atm the impl, seem to be able to either:

  1. build a LoopNest with a body (with or without results / iter args)
  2. build a LoopNest without a body (with or without results / iter args)
  3. inline the body without creating a LoopNest.

This API is doing too much and splitting 3. out would be the much cleaner solution IMO.

csigg updated this revision to Diff 475435.Nov 15 2022, 5:20 AM
csigg edited the summary of this revision. (Show Details)

Clean up test TODO.

csigg added a comment.Nov 15 2022, 5:24 AM

This API is doing too much and splitting 3. out would be the much cleaner solution IMO.

That is your opinion, but the API is currently used for all three cases and avoiding the foot-gun for 3. is an orthogonal improvement.

I noticed only now that this change allows cleaning up a TODO item in the test.

ftynse added a subscriber: ftynse.Nov 22 2022, 1:57 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/SCF/IR/SCF.h
64–65

The point of using std::vector instead of SmallVector was to avoid excessive copying of stack-allocated data since LoopNest instances are returned by value.

66–67

How about changing getResults to getResultsOr(ValueRange)? The latter would work similarly to Optional::getValueOr and return the argument when no loops were created. This seems like it could satisfy the use case without changing how the builder works.

Sync'd offline with Alex who authored this builder.

My objection is I view buildX that 1. does not create X and 2. does not fail but 3. does something else as problematic.
We discussed an analogy: 0-D load is a corner case but still creates a load; 0-D loop nest OTOH will not create a loop nest so we need to put a conditional somewhere.
I am advocating to put the conditional at the client rather than hide it inside the builder because of my point just above.

OTOH Alex justly remarked that scf::buildLoopNest is not the same as scf::buildScfForOps and that it builds some abstract loop nest that we are ok defining as "empty loop nest with the builder called once".

Leaving @ftynse with the approval decision once this LGTH.

nicolasvasilache resigned from this revision.Nov 22 2022, 2:25 AM
nicolasvasilache added a reviewer: ftynse.
csigg added a comment.Nov 28 2022, 7:48 AM

Thanks for reviewing and sorry for the delay. Just to be clear, although I think it's a minor improvement, I'm not attached to this change. If you prefer to keep things as is, I will just abandon it.

mlir/include/mlir/Dialect/SCF/IR/SCF.h
64–65

I don't think the extra copies matter. Both Value and scf::ForOp are just a pointer.

66–67

That doesn't really help in e.g. TestTensorTransforms.cpp or my use case because there is no convenient way to access the body builder return value.

ftynse accepted this revision.Nov 29 2022, 3:26 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/SCF/IR/SCF.h
66–67

Good point.

This revision is now accepted and ready to land.Nov 29 2022, 3:26 AM
This revision was automatically updated to reflect the committed changes.