This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] NFC. Introduce mlir::hasEffect and refactor usages dialect util
ClosedPublic

Authored by bondhugula on Aug 18 2022, 2:25 AM.

Details

Diff Detail

Event Timeline

bondhugula created this revision.Aug 18 2022, 2:25 AM
bondhugula requested review of this revision.Aug 18 2022, 2:25 AM

Update comment.

bondhugula added a reviewer: arnab-oss.
rriddle accepted this revision.Aug 22 2022, 12:46 AM
This revision is now accepted and ready to land.Aug 22 2022, 12:46 AM

I think this change alters the semantics, and won't work if the op has deallocate + some other effects on val. Those type of ops still qualify as dealloc ops.

I think this change alters the semantics, and won't work if the op has deallocate + some other effects on val. Those type of ops still qualify as dealloc ops.

Yes, it does tighten the semantics. An op that has, say, a read effect and a dealloc effect on the same value will no longer be treated as a dealloc by this utility. We probably need to introduce hasEffect<....>(user, value) and use that.

mehdi_amini requested changes to this revision.Aug 22 2022, 9:23 AM

We probably need to introduce hasEffect<....>(user, value) and use that.

This is basically the code implemented in-line right now, right?

This revision now requires changes to proceed.Aug 22 2022, 9:23 AM

We probably need to introduce hasEffect<....>(user, value) and use that.

This is basically the code implemented in-line right now, right?

Yes, that's exactly what it is.

We probably need to introduce hasEffect<....>(user, value) and use that.

This is basically the code implemented in-line right now, right?

Updated now @mehdi_amini PTAL, the new utlity.

bondhugula retitled this revision from [MLIR] Improve check in memref dialect util findDealloc to [MLIR] NFC. Introduce mlir::hasEffect and refactor usages dialect util.
bondhugula edited the summary of this revision. (Show Details)

Update to switch check to a new utility for the old code. This makes it NFC.

mehdi_amini accepted this revision.Sep 3 2022, 2:54 AM
This revision is now accepted and ready to land.Sep 3 2022, 2:54 AM

LGTM
Thanks for implementing the suggested changes.