This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] hoist loop invariant tensor loads in sparse compiler
ClosedPublic

Authored by aartbik on Dec 2 2020, 7:45 PM.

Details

Summary

After bufferization, the backend has much more trouble hoisting loop invariant
loads from the loops generated by the sparse compiler. Therefore, this is done
during sparse code generation. Note that we don't bother hoisting derived
invariant expressions on SSA values, since the backend does that very well.

Still TBD: scalarize reductions to avoid load-add-store cycles

Diff Detail

Event Timeline

aartbik created this revision.Dec 2 2020, 7:45 PM
aartbik requested review of this revision.Dec 2 2020, 7:45 PM
penpornk accepted this revision.Dec 7 2020, 1:12 AM

I'm sorry for the delay! The generated MLIR code looks good to me.

mlir/lib/Dialect/Linalg/Transforms/Sparsification.cpp
68

Nit: It seems inner-level comments should start with // instead of ///. If this is true, please fix. Same for lines 71-72 and 86.

72

Thank you for more code comments!

76

Nit: s/consist/consists/

515

Nit: Why the question mark?

565

Just curious why you refactored this. Do you plan to expand this function in the future?

994

This is clever. :) I was wondering why if (!codegen.loops[idx]) in line 606 would work before I reach this point.

This revision is now accepted and ready to land.Dec 7 2020, 1:12 AM
aartbik marked 5 inline comments as done.Dec 7 2020, 9:38 AM

Thanks for the review!

mlir/lib/Dialect/Linalg/Transforms/Sparsification.cpp
68

For classes/struct, the methods/fields still get the /, inside code it gets
But now that you point this out, I see I missed a few /// cases! Thanks!

515

Ah, because I test that in the next line. Rephrased to be more clear.

565

Yes! The next or after next CL will be vectorization, and for that it is nice to have all the buffer references in a method.

994

Yes, before entering the next loop at the same level, we need to make sure none of the hoisting is valid anymore. So by keeping this "invariant" (pun intended) during the recursion, it all works well together.

aartbik updated this revision to Diff 309957.Dec 7 2020, 10:46 AM
aartbik marked 4 inline comments as done.

addressed comments

aartbik updated this revision to Diff 309978.Dec 7 2020, 11:56 AM

moved to main (I think....)

This revision was landed with ongoing or failed builds.Dec 7 2020, 12:00 PM
This revision was automatically updated to reflect the committed changes.