Page MenuHomePhabricator

[mlir] Remove over specified memory effects
ClosedPublic

Authored by youngar on Jan 14 2021, 2:19 AM.

Details

Summary

The standard and gpu dialect both have alloc operations which use the
memory effect MemAlloc. In both cases, it is specified on both the
operation itself and on the result. This results in two memory effects
being created for these operations. When MemAlloc is defined on an
operation, it represents some background effect which the compiler
cannot reason about, and inhibits the ability of the compiler to
remove dead std.alloc operations. This change removes the uneeded
MemAlloc effect from these operations and leaves the effect on the
result, which allows dead allocs to be erased.

There is the same problem, but to a lesser extent, with MemFree, MemRead
and MemWrite. Over-specifying these traits is not currently inhibiting
any optimization.

Diff Detail

Event Timeline

youngar created this revision.Jan 14 2021, 2:19 AM
youngar requested review of this revision.Jan 14 2021, 2:19 AM

LGTM for the sparse changes, it removes the index array when the sparse kernel does not need it

rriddle accepted this revision.Jan 14 2021, 12:29 PM

Thanks!

mlir/include/mlir/Dialect/GPU/GPUOps.td
834

Can you remove the <DefaultResource> here too? It isn't necessary.

This revision is now accepted and ready to land.Jan 14 2021, 12:29 PM
youngar updated this revision to Diff 316763.Jan 14 2021, 1:13 PM

[mlir][gpu] Remove unneeded DefaultResource

DefaultResource is implied.

Should I get more reviews, or should I arc land this now?

Should I get more reviews, or should I arc land this now?

If you have commit access, feel free to go ahead and land. If not, let me know and I can land it for you.

youngar marked an inline comment as done.Jan 14 2021, 7:22 PM

I manually rebased+pushed the commit to the github main branch. Will phabricator automatically close this? Did I do the something wrong?

mehdi_amini closed this revision.Jan 14 2021, 8:18 PM

Phab linked the commit as "added a commit" to this revision, I'm not sure why it didn't close it...

Thanks for the help and reviews everyone!

Thanks for cleaning this up!