Page MenuHomePhabricator

[mlir] Add hoisting of transfer ops in affine loops
AcceptedPublic

Authored by jsetoain on Mon, Nov 7, 4:29 PM.

Details

Summary

The only way to do this with the current hoisting strategy is by
lowering Affine to Scf first, but that prevents further passes on
Affine.

Diff Detail

Event Timeline

jsetoain created this revision.Mon, Nov 7, 4:29 PM
jsetoain updated this revision to Diff 476189.Thu, Nov 17, 11:18 AM

Make utility function names more specific

jsetoain updated this revision to Diff 476213.Thu, Nov 17, 12:54 PM

clang-format

jsetoain published this revision for review.Thu, Nov 17, 1:14 PM
bondhugula accepted this revision.Wed, Nov 23, 5:25 AM

This looks great to me - thanks! Mostly minor comments.

mlir/include/mlir/Dialect/Affine/Utils.h
336 ↗(On Diff #476213)

Nit: doesn't
Similarly, apostrophes missing below.

mlir/lib/Dialect/Linalg/Transforms/Hoisting.cpp
521–522

dyn_cast here to avoid an extra check below.

528

dyn_cast here again.

534

You should actually check earlier and advance the walk if it's something other than affine.for or scf.for at line 428; either that or assert here with an extra else or use cast above for AffineForOp in the conditional.

mlir/lib/Dialect/SCF/Utils/Utils.cpp
39–40

This should've been in the mlir::scf namespace. You can update it in this PR itself if you wish.

83

SCFForLoop -> SCFFor or SCFLoop for conciseness.

This revision is now accepted and ready to land.Wed, Nov 23, 5:25 AM
jsetoain updated this revision to Diff 477519.Wed, Nov 23, 8:52 AM
jsetoain marked 6 inline comments as done.

Address comments

jsetoain marked an inline comment as not done.Wed, Nov 23, 8:59 AM

Thanks for the review!

mlir/lib/Dialect/Linalg/Transforms/Hoisting.cpp
521–522

I haven't found a way to dyn_cast the result of the dyn_cast. I'm not very familiar with llvm's static polymorphism constructs, so I'm not sure what the problem is, but I get errors like:

error: invalid cast of an rvalue expression of type ‘std::nullptr_t’ to type ‘llvm::CastInfo<mlir::scf::ForOp, mlir::LoopLikeOpInterface, void>::CastReturnType’ {aka ‘mlir::scf::ForOp&’}

Alternatively, I can write: dyn_cast<scf::ForOp>(transferRead->getParentOp()) but I'm not sure it's worth the loss of clarity.

mlir/lib/Dialect/SCF/Utils/Utils.cpp
39–40

Indeed, this has been bugging me since I submitted the patch. I agree, it would be better if instead of using namespaces in the function names, we placed these utility functions in their respective namespaces. It's a straightforward NFC, but it would splash everywhere; I will push a separated patch as soon as I'm done with this review.

bondhugula added inline comments.Wed, Nov 23, 9:07 AM
mlir/lib/Dialect/Linalg/Transforms/Hoisting.cpp
521–522

Please see https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates.

Use dyn_cast instead of isa + cast, i.e.,

if (auto scfFor = dyn_cast<scf::ForOp>(loop)) {
  ...
mlir/lib/Dialect/SCF/Utils/Utils.cpp
80

innermost

bondhugula requested changes to this revision.Wed, Nov 23, 9:12 AM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/Utils/Utils.cpp
1877–1941 ↗(On Diff #477519)

Can we templatize this function and make it work for both AffineForOp and scf::ForOp? You'll need YieldOpTy (can be scf::YieldOp and AffineYieldOp) and ForOpTy (can be scf::ForOp or AffineForOp). Other than the op creation calls, isn't everything else the same?

This revision now requires changes to proceed.Wed, Nov 23, 9:12 AM
jsetoain updated this revision to Diff 477603.Wed, Nov 23, 2:13 PM

Try dyn_cast

mlir/lib/Dialect/Linalg/Transforms/Hoisting.cpp
521–522

I believe I didn't explain myself very well there. I understand how LLVM's RTTI is supposed to work, I don't understand why it's not working. I'm getting that error in my previous comment, but I don't understand enough about the implementation to figure out why.

I'm going to push the change anyway. Best case scenario, it's something wrong with my compiler. Worst case scenario, it will leave a nice log of the error I'm seeing.

mravishankar requested changes to this revision.Wed, Nov 23, 2:56 PM
mravishankar added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Hoisting.cpp
521–522

I am missing overall context, but you cant dyn_cast result of the dyn_cast....

IIUC what is being suggested here is

if (auto scfForOp = dyn_cast<scf::ForOp>(loop)) {
  ....
} else if (auto affineForOp = dyn_cast<AffineForOp>(loop)) {

}

or better yet, use TypeSwitch

return TypeSwitch<Operation *, WalkResult>(loop)
  .Case<scf::ForOp>([](scf::ForOp scfForOp) {...})
  .Case<AffineForOp>([](AffineForOp affineForOp) {...})
  .Default([](Operation *) { return WalkResult::interrupt(); }

(and move the loop.erase into the if in first case or callback for the Case in the second example).

This revision now requires changes to proceed.Wed, Nov 23, 2:56 PM
jsetoain updated this revision to Diff 477649.Wed, Nov 23, 4:28 PM

Address comments (1/n)

jsetoain marked 3 inline comments as done.Wed, Nov 23, 4:34 PM
jsetoain added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Hoisting.cpp
521–522

That's what I suspected, and the problem is that loop is the result of a dyn_cast, so it can't be dyn_cast-ed again. Thankfully, TypeSwitch does the trick.

mravishankar added inline comments.Wed, Nov 23, 4:49 PM
mlir/lib/Dialect/Linalg/Transforms/Hoisting.cpp
521–522

dyn_cast, etc. works only on Operation *. When you define TypeSwitch<Operation *, ....>, the LoopLikeInterface op gets implicitly cast to Operation *, and then TypeSwitch works the same way as dyn_cast. So with the if version you would need if (auto scfForOp = dyn_cast<scf::ForOp>(loop.getOperation()))

mravishankar resigned from this revision.Wed, Nov 23, 4:52 PM

I dont have further comments on this patch. This only landed on my radar cause of this statement `I'm going to push the change anyway. Best case scenario, it's something wrong with my compiler. Worst case scenario, it will leave a nice log of the error I'm seeing.`
I might have mis-judged the sentiment, but pushes that could cause failures would cause pain downstream (and would probably get reverted anyway). Apologies if I misread this.

jsetoain updated this revision to Diff 477769.Thu, Nov 24, 6:22 AM
jsetoain marked 2 inline comments as done.

Templatize function

jsetoain added a comment.EditedThu, Nov 24, 6:23 AM

I dont have further comments on this patch. This only landed on my radar cause of this statement `I'm going to push the change anyway. Best case scenario, it's something wrong with my compiler. Worst case scenario, it will leave a nice log of the error I'm seeing.`
I might have mis-judged the sentiment, but pushes that could cause failures would cause pain downstream (and would probably get reverted anyway). Apologies if I misread this.

My apologies. English is not my native language, I didn't mean it like that, but I see where the confusion may have come from. What I was trying to say is "I will update this patch in revision, let the automatic checks fail where they fail for me locally, and then I can point to those logs to explain my problem". You can rest assure, I never had the intention to land this patch until all issues are clarified and everyone is happy with my code, and I would never land a patch that doesn't pass all tests in my machine :-) I appreciate the clarification on dyn_cast issue, this was a new one for me, thanks!

mlir/lib/Dialect/Affine/Utils/Utils.cpp
1877–1941 ↗(On Diff #477519)

That and the access to the YieldOp operands (which I find strange). I considered the template early on, but was not convinced with the result. I'm uploading my templated version, let me know what you think. My issues are:

  1. It only really works for scf::ForOp and AffineForOp
  2. I'm getting away with not making SCF dependent on Affine only because that's the only alternative to scf::ForOp. It's creating a non-explicit dependency that I don't like.
  3. The whole thing should probably live somewhere else, but at the same time, this is code depends on a lot of for loop-related functions and methods that don't work for all possible loops.
  4. It only "coincidentally" works for both Affine and SCF fors because they have methods with the same identifiers. It feels like abusing templates to avoid copying a couple dozen lines of code.

That's why I originally scrapped it and decided in favour of total isolation. Whether this is preferable or not I will leave it up to you, you have the experience to judge :-)

jsetoain updated this revision to Diff 477770.Thu, Nov 24, 6:26 AM

Remove unnecessary empty line

bondhugula accepted this revision.Thu, Nov 24, 12:31 PM

LGTM - thanks.

This revision is now accepted and ready to land.Thu, Nov 24, 12:31 PM
bondhugula added inline comments.Thu, Nov 24, 12:33 PM
mlir/include/mlir/Dialect/SCF/Utils/Utils.h
57

This much C++ code in the header file isn't okay. I understand this was added here because it's a template function. Instead, you can just define it out of line and explicitly instantiate for both scf::ForOp and AffineForOp in the C++ file (two extra lines right below the definition).